FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add itertools.batched Support by Poppro · Pull Request #5209 · RustPython/RustPython · GitHub

/ RustPython Public

Add itertools.batched Support - #5209

Merged
youknowone merged 7 commits into
RustPython:mainfrom
Poppro:itertools-batched
Apr 6, 2024
ConversationCommits (7)ChecksFiles changed
Merged

Add itertools.batched Support#5209
youknowone merged 7 commits into
RustPython:mainfrom
Poppro:itertools-batched

Conversation

Poppro commented Mar 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adds the itertools.batched function new in python 3.12

https://docs.python.org/3/library/itertools.html#itertools.batched

youknowone reviewed Mar 28, 2024
View reviewed changes

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you so much! Could you also update test_itertools from CPython 3.12 to ensure this patch is compliant its spec? The related issue is #5104

Poppro commented Mar 29, 2024

Copy link
Copy Markdown
Contributor Author

@youknowone

Thank you so much! Could you also update test_itertools from CPython 3.12 to ensure this patch is compliant its spec? The related issue is #5104

I went ahead and added the 3.12 itertools_test update to this PR.

Notes:

  1. This caught an out-of-order exception throw in the batched implementation. Corrected now.
  2. I had to mark a number of previously passing tests as ignored, since they now expect DepreciatedWarnings to be thrown.

youknowone mentioned this pull request Mar 30, 2024
Closed
youknowone requested changes Mar 30, 2024
View reviewed changes

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's a good point of tests 😄 Thank you!

Comment thread vm/src/stdlib/itertools.rs Outdated
Comment thread vm/src/stdlib/itertools.rs Outdated
if n.lt(&BigInt::one()) {
return Err(vm.new_value_error("n must be at least one".to_owned()));
}
let n = n.to_usize().unwrap();

youknowone Mar 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What happens if n is a negative number? That will be a python error rather than panic (by unwrap).
By the error type and message, you might want to change the type of BatchedNewArgs::n to usize or any unsigned type. Otherwise making an ok_or_else chain will be a good choice.

Poppro Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Doesn't the conditional on line 1985 ensure that n will not be a number less than 1?

youknowone Apr 1, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's right, thank you! How about n > usize::MAX?
Since unwrap is one of main source of uncontrolled panic, we prefer to unsure they don't have edge cases and a bit picky about it.
If that's logically not able to be triggered, using expect() with reasoning rather than unwrap() will be very helpful to provide the reasoning and a debug hint for future regression.

Poppro Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Great point! CPython throws the following error in such case: OverflowError: Python int too large to convert to C ssize_t.

I've modified the unwrap to instead use ok_or and return an overflow error in this case.

Poppro commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

Build failures seem unrelated?

fanninpm commented Apr 5, 2024

Copy link
Copy Markdown
Contributor

Build failures seem unrelated?

The macOS failure in test_re seems unrelated. However, can you please run cargo fmt to see if that fixes the other failure? (And, for good measure, consider running cargo clippy?)

Poppro commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

Build failures seem unrelated?

The macOS failure in test_re seems unrelated. However, can you please run cargo fmt to see if that fixes the other failure? (And, for good measure, consider running cargo clippy?)

Here's the results - no changes or issues generated

youknowone approved these changes Apr 6, 2024
View reviewed changes

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you so much!

youknowone merged commit ae72316 into RustPython:main Apr 6, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL