Skip to content
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

GroupMap: add fold_with (2) more generic init #785

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

phimuemue
Copy link
Member

Hi, this completes #778

@Philippe-Cholet Could you please approve this? Apparently I cannot simply push anymore to the repo.

@jswrenn I ran into this problem twice now. Am I doing something wrong? Is there a way to allow fixups like this for maintainers?

This is a generalization of `fold` which takes a function rather than a
value, which removes the need for a `Clone` bound. `fold` is implemented
in terms of `fold_with`.
@tamird
Copy link
Contributor

tamird commented Oct 20, 2023

It seems a bit strange to accept the value since init is only called for the first key-value pair in a group. The value that the closure will be called with is not guaranteed to be deterministic AFAIK.

@Philippe-Cholet
Copy link
Member

@tamird I agree it's weird to add the value parameter, I'm not fond of it. But I guess it's not a big pain to ignore it by adding ,_ to the closure. And maybe there is an usage for it.

@phimuemue Can't you approve them yourself? In the "Files changed" page: click "Review changes" button, check "Approve" box, valid by clicking "Submit review" button. That's what I would do. I'd like to know where you are blocked if you are.

@phimuemue
Copy link
Member Author

@Philippe-Cholet I already tried this. "Approve" is disabled (seems I cannot review my own PRs).

@phimuemue
Copy link
Member Author

phimuemue commented Oct 20, 2023

It seems a bit strange to accept the value since init is only called for the first key-value pair in a group. The value that the closure will be called with is not guaranteed to be deterministic AFAIK.

The whole function is niche - and as such I'd make it as generic as possible (as long as it does not incur runtime overhead). val is deterministic: the iterator's first item associated with its key.

@phimuemue phimuemue added this pull request to the merge queue Oct 20, 2023
Merged via the queue into rust-itertools:master with commit 7a6c1ef Oct 20, 2023
8 checks passed
@phimuemue phimuemue deleted the more_generic_init branch October 20, 2023 16:22
@Philippe-Cholet
Copy link
Member

@phimuemue I previously successfully merged two PRs (by approving) that I did not create but where I committed to. I guess the key point is that you created the PR.

@Philippe-Cholet Philippe-Cholet added this to the next milestone Nov 3, 2023
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants