Add itertools.batched Support #5209
Conversation
There was a problem hiding this comment.
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:
|
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Doesn't the conditional on line 1985 ensure that n will not be a number less than 1?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Build failures seem unrelated? |
The macOS failure in |
Here's the results - no changes or issues generated |
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
1bbbd3e to
90ab0e3
Compare
Adds the itertools.batched function new in python 3.12
https://docs.python.org/3/library/itertools.html#itertools.batched