-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Proposal: fold_self
and try_fold_self
for Iterators
#65222
Conversation
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
This is a retry of #60103, which I wasn't able to work on before it was closed for inactivity |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
On naming, We even have this helper function locally, used in rust/src/libcore/iter/traits/iterator.rs Lines 2958 to 2970 in 3fa9554
That implementation also matches how I would write this, rather than using |
I saw that, but it didn't seem to be a very "rusty" name. I'm happy to be convinced, but my experience with the rust stdlib has been that it tends to use the more "plain" name in public methods.
Can you elaborate on why? This seems like a platonically ideal use case for |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Sure, plain names are preferred, but I didn't know what
It's also a good use case for the |
I would tend to agree that |
Hi @Lucretiel 👋 Thanks for the PR! Do you have any examples of real-world code that you think is improved by adding these new methods? |
Changed to
Changed to
I wish I could find them; I originally wrote this PR way back in spring, and I can't recall what I was doing back then. I think it was a statistics thing? Certainly it applies in any case where you need to compare each element in the collection; we have To be honest, I think that |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
That error appears to be related to |
Ping from triage. @KodrAus any updates on this? Thanks. |
If there is no proposed use case for For the tidy error, barring splitting the file now, there is an opt-out of the warning you can apply, adding I don't think We have two differences between
So we could take either 1 or 2 as starting point for a name. |
Please try to split the file in a PR before this one. We have the tidy lint for a reason. |
There's very little room to split that file -- all but about 50 lines is consumed by the atypically huge |
Aw -- fair point; well lets use the exception in this case then. |
Ping from triage - the PR has sat idle for a week - and the build has failed. Any updates on this? |
Ping from triage - the PR has sat idle for a week - and the build has failed. Any updates on this? |
The build failure here is because of the tidy check. So what this PR effectively implements is an alternative to: // where f: Fn(Self::Item, Self::Item) -> Self::Item
iter.next().map(|a| iter.fold(a, f)); This seems like a useful method, since users trying to solve this problem might find themselves fussing around with multiple expressions to try thread that first value through, but it doesn't seem to complete any logical gaps in the current set of |
Ping from triage - can anyone answer the question from @KodrAus so this can be moved forward? |
Yes, fold1 in itertools is equivalent |
Ping from triage - |
Ping from triage - |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@Lucretiel any updates? |
Triaged |
- Added `Iterator::fold_first`, which is like `fold`, but uses the first element in the iterator as the initial accumulator - Includes doc and doctest - Rebase commit; see rust-lang#65222 for details Co-Authored-By: Tim Vermeulen <[email protected]>
heck wtf |
okay fixed the doctests and the git-weirdness now let's see what happens |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors try |
⌛ Trying commit 36e17c78b69cfb657ce75b0c59a25cb3030711e7 with merge e1c4b614947b45173b0640bff464bf1f0135f477... |
💔 Test failed - checks-azure |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r=kodrAus rollup |
📌 Commit 268408f has been approved by |
Rollup of 4 pull requests Successful merges: - rust-lang#65222 (Proposal: `fold_self` and `try_fold_self` for Iterators) - rust-lang#69887 (clean up E0404 explanation) - rust-lang#70068 (use "gcc" instead of "cc" on *-sun-solaris systems when linking) - rust-lang#70470 (Clean up E0463 explanation) Failed merges: r? @ghost
☔ The latest upstream changes (presumably #70474) made this pull request unmergeable. Please resolve the merge conflicts. |
Wait, should this have been merged? I thought there were still outstanding issues that needed resolving |
@Lucretiel the only issue was regarding the doc tests which was fixed. Anything else? |
This pull request proposes & implements two new methods on Iterators:
fold_self
andtry_fold_self
. These are variants offold
andtry_fold
that use the first element in the iterator as the initial accumulator.Let me know if a public feature like this requires an RFC, or if this pull request is sufficient as place for discussion.