Skip to content

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

Merged
gemelen merged 5 commits into
typelevel:masterfrom
opencastsoftware:cats-hash-set
Aug 22, 2022
Merged

Add cats.kernel.Hash-based CHAMP HashSet#533
gemelen merged 5 commits into
typelevel:masterfrom
opencastsoftware:cats-hash-set

Conversation

@DavidGregory084

@DavidGregory084 DavidGregory084 commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread tests/src/test/scala/cats/collections/DefaultScalaCheckPropertyCheckConfig.scala
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)

@DavidGregory084 DavidGregory084 Jul 27, 2022

Copy link
Copy Markdown
Contributor 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

gemelen commented Jul 28, 2022

Copy link
Copy Markdown
Collaborator

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

armanbilge commented Jul 28, 2022

Copy link
Copy Markdown
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
johnynek
johnynek reviewed Jul 28, 2022
View reviewed changes
Comment thread core/src/main/scala/cats/collections/HashSet.scala Outdated
DavidGregory084
DavidGregory084 commented Jul 28, 2022
View reviewed changes
Comment thread core/src/main/scala-2.13+/cats/collections/compat/HashCompat.scala
DavidGregory084
DavidGregory084 commented Jul 29, 2022
View reviewed changes
Comment thread core/src/main/scala/cats/collections/HashSet.scala Outdated
@DavidGregory084
DavidGregory084 dismissed stale reviews from johnynek and gemelen via aaa8a0a August 2, 2022 12:35
@codecov-commenter

codecov-commenter commented Aug 2, 2022

Copy link
Copy Markdown

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

DavidGregory084 commented Aug 22, 2022

Copy link
Copy Markdown
Contributor 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