-
Notifications
You must be signed in to change notification settings - Fork 295
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 .flatten_ok() #527
add .flatten_ok() #527
Conversation
src/flatten_ok.rs
Outdated
} else { | ||
self.inner = None; | ||
} | ||
} else { |
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.
Do you really need this else
? The only case where it isn't executed is if self.inner
is Some(_)
and inner.next()
is also Some(_)
, but in that case you have an early return
. I would execute the contents of the else
in any case, both if the if let
succeeds or not.
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.
@SkiFire13 Your point is sound. In fact, we shouldn't even need to assign it to None
in that case as it will just automatically fall through. That might hurt performance in the case where next()
is called on an empty iterator repeatedly, but I don't think that actually matters. So yes, let me make that change.
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 think setting it to None
might have a reason if you consider consistency in case next
is called after the iterator ended. Should it try to advance the last inner iterator? Or the outer iterator? Currently the stdlib fuse both of them, but I guess it would be reasonable if you advance the outer iterator (in that case you need to set the self.inner
to 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.
I realize now that I should not change setting the inner to None
as it might incur a performance penalty when hitting several Err
values in a row.
A couple of other things you could also add:
|
src/flatten_ok.rs
Outdated
fn clone(&self) -> Self { | ||
Self { | ||
iter: self.iter.clone(), | ||
inner: self.inner.clone(), | ||
} | ||
} |
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 could use clone_fields
for consistency here.
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.
Thank you. Fixed and will push shortly.
I don't think we can implement I could add a specialized implementation of fold, but that could be done in another PR if someone runs into a performance issue with it. I think it should be alright as-is.
I can add those.
I couldn't think of a way to implement I can definitely implement
I think I will just allow the user to call |
I think you can use the current inner iterator's
I think it's good practice to provide specialized implementations of
The stdlib does exactly that. I don't think it's a big deal if the iterator is larger since it's supposed to be short lived and probably inlined. The additional check shouldn't matter that much since if you never call |
@SkiFire13 Okay, in that case I will add a size_hint impl. It wont help in the case that the iterator hasn't already had |
The |
@SkiFire13 Okay, I believe I am done now. I did not add tests for |
Okay, I just looked over my code and I think there may be an issue if the outer iterator is not fused. Notably, the call to |
I actually made the "fix" which fuses the outer iterator, and then realized it's technically still correct the way it is because if the outer iterator isn't fused, neither is Branch: https://github.com/vadixidav/itertools/tree/fused_flatten_ok |
I guess that's why the stdlib decided to always fuse the outer iterator too.
You could use the |
@SkiFire13 That's alright, I think this is good to go if you think so. If you do want it to be fused to merge though let me know, but for now its only fused if the input iterator is fused. |
For merging you should ask @phimuemue or @jswrenn |
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.
Hi there. I think there's really progress been made on this. Thank you for taking the time.
Unfortunately, I do not have enough capacity to review pull requests that ample right now.
However, one thing that might simplify review: As far as I can see, this somewhat resembles FlattenCompat
- as suggested here (btw thanks @SkiFire13). Maybe we could document this somewhere and even adapt FlattenCompat
s structure (i.e. naming of fields, match
vs if let
) so that a reviewer can basically diff your implementation against std
's FlattenCompat
.
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
f.debug_struct("FlattenOk") | ||
.field("iter", &self.iter) | ||
.field("inner_front", &self.inner_front) | ||
.field("inner_back", &self.inner_back) | ||
.finish() | ||
} |
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 you could use debug_fmt_fields
for consistency here.
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.
Thanks for the contribution! (And sorry for taking so long to merge it!)
bors r+
Build succeeded: |
Closes #522