Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add itertools.batched Support #5209

Merged
merged 7 commits into from
Apr 6, 2024
Merged

Conversation

Poppro
Copy link
Contributor

@Poppro Poppro commented Mar 28, 2024

Adds the itertools.batched function new in python 3.12

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

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Poppro commented Mar 29, 2024

@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.

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

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

vm/src/stdlib/itertools.rs Outdated Show resolved Hide resolved
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();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Poppro commented Apr 5, 2024

Build failures seem unrelated?

@fanninpm
Copy link
Contributor

fanninpm commented Apr 5, 2024

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
Copy link
Contributor Author

Poppro commented Apr 5, 2024

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?)

Screenshot 2024-04-05 at 6 47 57 PM

Here's the results - no changes or issues generated

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@youknowone youknowone merged commit ae72316 into RustPython:main Apr 6, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants