-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement itertools.accumulate #1372
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
Conversation
11f19c3
to
708fd9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; I've left a few comments. Thanks for contributing!
vm/src/stdlib/itertools.rs
Outdated
binop: binop.unwrap_or_else(|| vm.get_none()), | ||
acc_value: RefCell::from(vm.get_none()), | ||
} | ||
.into_ref(vm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use into_ref_with_type()
here and make the return type PyResult<PyItertoolsAccumulate>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for pointing out!
vm/src/stdlib/itertools.rs
Outdated
struct PyItertoolsAccumulate { | ||
iterable: PyObjectRef, | ||
binop: PyObjectRef, | ||
acc_value: RefCell<PyObjectRef>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe have this as an Option
rather than using None
as "not yet started iterating", because the binop function could return None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this wasn't carefully thought out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for me
Thanks @coolreader18
#1361
This is an implementation of the Python standard library function
itertools.accumulate
. I also included a couple of very basic tests.