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
Added Itertools::dedup_with_count()
and Itertools::dedup_by_with_count()
#423
Conversation
@jswrenn Can you take a look when you have some time? |
Sorry for the delay and for the contribution! My only nit is the name—it suggests that (This is yet another case where having rust-lang/rfcs#2584 would be helpful!) |
Hi there! Should we try to implement this in terms of (a generalized) As far as I can see, it (naturally) shares quite a bit of implementation with If we actually want to implement it in terms of |
I think it makes sense to start the function name with the fundamental thing that is going on, which is deduplication. The count is the extra metadata. I also like how an alphabetical order of the names of the functions naturally group these functions together (and also that autocompletion on Yeah, it makes sense to try to re-use code. I've re-wrote it in terms of |
Kudos for going with Imho, we should still strive to unify the implementations further, but your observation
made me think why we even have Doing so, however, may be a bit too much for this ticket. |
Yes, I think that's perfectly possible. I can look into it, but, as you suggest, I prefer to do that in a later PR. |
I sketched an implementation for this in #440. As stated there, I will rebase it if this PR is accepted. |
Looks good to me! Thanks! bors r+ |
Build succeeded: |
440: Implement DedupBy in terms of Coalesce r=jswrenn a=phimuemue During the discussion of #423 (comment), we found that `DedupBy`/`DedupByWithCount` could actually be implemented in terms of `Coalesce`. This simplifies the implementation quite a bit and lets derived adaptors inherit specializations. * Implement `fold` on `Coalesce` so that all the adaptors derived from it inherit the specialization (currently, only `DedupBy` has this specialization). * Introduce `CoalescePredicate` (similar to `DedupPredicate`) to allow for different customizations of `Coalesce`. * Introduce parametrizable `CoalesceBy`: Base for `Coalesce`, `DedupBy`, `DedupByWithCount`. * Implement `DedupBy`/`DedupByWithCount` in terms of `CoalesceBy` (using particular `impl`s). * At last, the indirection through `CoalesceCore` is not needed anymore. Co-authored-by: philipp <descpl@yahoo.de>
450: Introduce module coalesce r=jswrenn a=phimuemue Hi! As #440 (thanks for reviewing and merging, btw) presumably conflicts with any existing PRs touching `coalesce` et al, I thought we could just as well seize the opportunity and move these types into an own module (so to speak as a starting point for the problem mentioned in #423 (comment)). I cut-pasted `Coalesce` et al into an own module, rearranged the code so that specialized types follow their generic variants, and ran `cargo fmt` on the new module. I suggest to import the `coalesce` types into `adaptors` similarly to what we already have for `multi_product`. Co-authored-by: philipp <descpl@yahoo.de>
Fixes #393.