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.

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

Move contain_ to UnorderedFoldable #4183

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

BalmungSan
Copy link
Contributor

No description provided.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Note that this will be source-breaking for anyone relying on something like import cats.syntax.foldable._ instead of a catch-all. I think this is probably ok?

core/src/main/scala/cats/syntax/unorderedFoldable.scala Outdated Show resolved Hide resolved
armanbilge
armanbilge previously approved these changes Apr 12, 2022
@satorg
Copy link
Contributor

satorg commented Apr 12, 2022

I'd suppose, it might be reasonable to add contains_ to the UnorderedFoldable type class itself. Because potentially it could be overridden with more efficient implementations when applicable.

@armanbilge
Copy link
Member

Good point, that may very well be the case when #4147 materializes :)

@BalmungSan
Copy link
Contributor Author

BalmungSan commented Apr 12, 2022

@satorg @armanbilge I move it to UnorderedFoldable as contains and the constains_ extension now forwards to the instance method.

I assume I should test it, but not sure what would be a good test?
All I can think of is ensuring it is consistent with the exists version.

@satorg
Copy link
Contributor

satorg commented Apr 12, 2022

I'd suggest two ways to test it: as you said, consistency with exists, and also consistency with Set#contains might be useful as well. I assume, Set implements UnorderedFoldable – is that correct?

@BalmungSan
Copy link
Contributor Author

@satorg I just pushed the tests, let me know what you think.

@satorg
Copy link
Contributor

satorg commented Apr 12, 2022

Note that this will be source-breaking for anyone relying on something like import cats.syntax.foldable._ instead of a catch-all. I think this is probably ok?

Well, seems that import cats.syntax.foldable._ makes available the syntax for UnorderedFoldable as well:

trait FoldableSyntax extends Foldable.ToFoldableOps with UnorderedFoldable.ToUnorderedFoldableOps {
   ???
}

So we should be safe there (not sure if it makes sense to write a test for that though)...

@BalmungSan BalmungSan force-pushed the fix-contains branch 3 times, most recently from f677361 to c057686 Compare April 13, 2022 14:01
@BalmungSan BalmungSan changed the title Move contain_ to UnorderedFoldableOps Move contain_ to UnorderedFoldable Apr 13, 2022
@satorg
Copy link
Contributor

satorg commented Apr 13, 2022

You may need to add the new laws created to the cats.laws.discipline.UnorderedFoldableTests. Otherwise these new laws will have no effect on real laws-checking tests.

@BalmungSan
Copy link
Contributor Author

You may need to add the new laws created to the cats.laws.discipline.UnorderedFoldableTests. Otherwise these new laws will have no effect on real laws-checking tests.

Oh, right, that explains why the original buggy law did not fail the tests.
Sorry about that, I was not familiar with how the laws are organized.

But I just pushed that change plus the two extra laws you proposed!

@BalmungSan BalmungSan force-pushed the fix-contains branch 5 times, most recently from 99c2f1f to fa6d79e Compare April 13, 2022 23:20
satorg
satorg previously approved these changes Apr 13, 2022
satorg
satorg previously approved these changes Apr 14, 2022
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

This looks great! Just some tiny nits :)

laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/UnorderedFoldable.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/syntax/unorderedFoldable.scala Outdated Show resolved Hide resolved
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

🎉

@armanbilge armanbilge merged commit fe40bc2 into typelevel:main Apr 14, 2022
@BalmungSan BalmungSan deleted the fix-contains branch April 14, 2022 15:18
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.

3 participants