Skip to content

Implement with_hasher adaptors#1007

Open
424ever wants to merge 7 commits intorust-itertools:masterfrom
424ever:with_hasher
Open

Implement with_hasher adaptors#1007
424ever wants to merge 7 commits intorust-itertools:masterfrom
424ever:with_hasher

Conversation

@424ever
Copy link
Copy Markdown

@424ever 424ever commented Dec 23, 2024

Closes #998

Implementations for the following methods:

  • duplicates_with_hasher
  • duplicates_by_with_hasher
  • unique_with_hasher
  • unique_by_with_hasher
  • all_unique_with_hasher
  • into_group_map_with_hasher
  • into_group_map_by_with_hasher
  • into_grouping_map_with_hasher
  • into_grouping_map_by_with_hasher
  • counts_with_hasher
  • counts_by_with_hasher

Copy link
Copy Markdown
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit, but otherwise this LGTM.

Comment thread src/duplicates_impl.rs Outdated
Comment thread src/lib.rs
@phimuemue
Copy link
Copy Markdown
Member

Two nits:

  • I'm a bit unhappy about duplicated tests. We can leave them as is and add a TODO that we'd ideally want to write all test cases once and apply them to both normal and _with_hasher functions automagically.

  • (Possibly over-nitpicky:) Should we rely on the fact that RandomState is the default BuildHasher or should we extract it so that std can do whatever it wants and we just adapt it? Could be done as follows:

    trait THashMap {
        type BuildHasher;
    }
    impl<K, V, S> THashMap for std::collections::HashMap<K, V, S> {
        type BuildHasher = S;
    }
    type StdDefaultBuildHasher = <std::collections::HashMap<(), ()> as THashMap>::BuildHasher;
    
    

@jswrenn Feel free to proceed as you deem appropriate.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 4, 2025

Codecov Report

❌ Patch coverage is 99.52830% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.86%. Comparing base (6814180) to head (194357b).
⚠️ Report is 185 commits behind head on master.

Files with missing lines Patch % Lines
src/grouping_map.rs 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   94.38%   93.86%   -0.53%     
==========================================
  Files          48       51       +3     
  Lines        6665     6633      -32     
==========================================
- Hits         6291     6226      -65     
- Misses        374      407      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jswrenn
Copy link
Copy Markdown
Member

jswrenn commented Jan 15, 2025

  • Should we rely on the fact that RandomState is the default BuildHasher or should we extract it so that std can do whatever it wants and we just adapt it?

It would be a breaking change for Rust to change its default hasher, so we're fine to rely on RandomState directly.

Comment thread tests/test_std.rs
@xtqqczze
Copy link
Copy Markdown
Contributor

@424ever Are you planning to continue with this PR?

@424ever
Copy link
Copy Markdown
Author

424ever commented Apr 15, 2026

@424ever Are you planning to continue with this PR?

Sorry for the late reply. Yes, I'll rebase this and clean it up a bit

@424ever
Copy link
Copy Markdown
Author

424ever commented Apr 15, 2026

Do I need to do something about the failing semver checks? Those aren't in any files this PR touches.

@xtqqczze
Copy link
Copy Markdown
Contributor

@phimuemue @jswrenn Can we get this in for 0.15?

@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented Apr 15, 2026

Do I need to do something about the failing semver checks?

The semver check failure is informational, and expected because there are changes to the public API.

@jswrenn
Copy link
Copy Markdown
Member

jswrenn commented Apr 15, 2026

@phimuemue @jswrenn Can we get this in for 0.15?

Yeah, sure! :)

@424ever 424ever requested a review from jswrenn April 16, 2026 19:26
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.

Provide with_hasher alternative adaptors

4 participants