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.

Sign up for GitHub

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

Jump to bottom

Add cats.kernel.Hash-based CHAMP HashSet #533

Merged
merged 5 commits into from
Aug 22, 2022
Merged

Add cats.kernel.Hash-based CHAMP HashSet #533

merged 5 commits into from
Aug 22, 2022

Conversation

DavidGregory084
Copy link
Member

This is a port of typelevel/cats#4185 to cats-collections.

@DavidGregory084 DavidGregory084 marked this pull request as draft July 27, 2022 13:16
DavidGregory084
DavidGregory084 commented Jul 27, 2022
View reviewed changes
tests/src/test/scala/cats/collections/DefaultScalaCheckPropertyCheckConfig.scala
@@ -27,6 +27,7 @@ object DefaultScalaCheckPropertyCheckConfig {
final val default: Parameters = Parameters.default
.withMinSuccessfulTests(if (BuildInfo.isJvm) 50 else 5)
.withMaxDiscardRatio(if (BuildInfo.isJvm) 5 else 50)
.withMaxSize(if (BuildInfo.isJvm) 10 else 5)
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to synchronise with cats - without it the tests take a really long time to run

@DavidGregory084 DavidGregory084 marked this pull request as ready for review July 27, 2022 14:04
@DavidGregory084 DavidGregory084 mentioned this pull request Jul 27, 2022
Closed
@gemelen
Copy link
Collaborator

gemelen commented Jul 28, 2022

As I understand, it's a latest version from Cats PR, so it's already went through a review regarding code and performance.
I have nothing to add to that review, as it's good to go from my point of view.

gemelen
gemelen previously approved these changes Jul 28, 2022
View reviewed changes
@armanbilge
Copy link
Member

That's definitely the case for the HashMap, I think the HashSet (this PR) has had less direct review although I David did a great job applying review feedback from the HashMap also to the HashSet.

If the plans are for cats-collections to 1.0 in the near future (I hope so!) we should make sure everything is carefully reviewed :) I do think this one is in great shape!

johnynek
johnynek previously approved these changes Jul 28, 2022
View reviewed changes
@DavidGregory084 DavidGregory084 dismissed stale reviews from johnynek and gemelen via aaa8a0a August 2, 2022 12:35
@codecov-commenter
Copy link

Codecov Report

Merging #533 (9fc386e) into master (78f9e4f) will not change coverage.
The diff coverage is n/a.

❗ Current head 9fc386e differs from pull request most recent head aaa8a0a. Consider uploading reports for the commit aaa8a0a to get more accurate results

@@      Coverage Diff      @@
##   master   #533   +/-   ##
=============================
=============================

Help us with your feedback. Take ten seconds to tell us how you rate us.

@DavidGregory084
Copy link
Member Author

Is there anything more to be done here? Anything I can do to move this forward?

@gemelen gemelen merged commit d9e4bc2 into typelevel:master Aug 22, 2022
@DavidGregory084 DavidGregory084 deleted the cats-hash-set branch August 22, 2022 08:57
@DavidGregory084 DavidGregory084 mentioned this pull request Aug 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnynek johnynek johnynek left review comments

@gemelen gemelen gemelen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DavidGregory084 @gemelen @armanbilge @codecov-commenter @johnynek