search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

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

Closed phimuemue closed 12 months ago

phimuemue commented 12 months ago

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?

tamird commented 12 months ago

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 commented 12 months ago

@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 commented 12 months ago

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

phimuemue commented 12 months ago

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.

Philippe-Cholet commented 12 months ago

@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.