itertools.compress #1400
itertools.compress #1400
Conversation
|
Hi @pitachips , thanks for this nice addition! Please check the clippy build on travis. This build should be fixed. The azure build is broken on master, so that probably has a cause unrelated to your change. |
vm/src/stdlib/itertools.rs
Outdated
| Err(e) => return Err(e), | ||
| } | ||
| } else { | ||
| self.next(vm) |
There was a problem hiding this comment.
Note that instead of recursion, you may want to prefer a loop here, to prevent hitting recursion limits.
There was a problem hiding this comment.
Sounds like a good idea. I will change it.
There was a problem hiding this comment.
@windelbouwman I made changes as suggested. Please have a look.
There was a problem hiding this comment.
This change looks good to me! I had one last comment, then I will merge this work.
vm/src/stdlib/itertools.rs
Outdated
| let data_obj = call_next(vm, &data_iter); | ||
|
|
||
| if verdict { | ||
| match data_obj { |
There was a problem hiding this comment.
This match could be change into a return data_obj statement, since both arms return from the function.
vm/src/stdlib/itertools.rs
Outdated
| let verdict = objbool::boolval(vm, sel_obj.clone())?; | ||
|
|
||
| let data_iter = get_iter(vm, &self.data)?; | ||
| let data_obj = call_next(vm, &data_iter); |
There was a problem hiding this comment.
You may want to add the question-mark operator to the end of this line, like this:
let data_obj = call_next(vm, &data_iter)?;What happens, is that in case of Err we return from the function, in case of Ok, we get the value into data_obj.
tests/snippets/stdlib_itertools.py
Outdated
|
|
||
| # itertools.compress | ||
| assert list(itertools.compress("ABCDEF", [1,0,1,0,1,1])) == list("ACEF") | ||
| assert list(itertools.compress("ABCDEF", [1,0,1,0,1,1])) == list("ACEF") |
There was a problem hiding this comment.
This test looks the same as the line above it?
61c93e3 to
1c8224d
Compare
vm/src/stdlib/itertools.rs
Outdated
| #[pymethod(name = "__next__")] | ||
| fn next(&self, vm: &VirtualMachine) -> PyResult { | ||
| loop { | ||
| let selector_iter = get_iter(vm, &self.selector)?; |
There was a problem hiding this comment.
One more comment, this call to get_iter might be redundant. It will get the iterator from an iterator. The iterator was already go at line 100, so I think it is sufficient to call the call_next method.
There was a problem hiding this comment.
That's so true. Got rid of the lines. Thanks!
|
Might want to rebase to kick the CI pipeline and see if it passes. |
|
Leaving a reference to #1361 so that we can see from that thread when it is merged. |
1c8224d to
44aedd5
Compare
44aedd5 to
239115f
Compare
implement itertools.compress