Skip to content

Add typeclasses for immutable.Seq #3620

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

Merged
merged 6 commits into from
Oct 1, 2020
Merged

Add typeclasses for immutable.Seq #3620

merged 6 commits into from
Oct 1, 2020

Conversation

PipelineBaron
Copy link
Contributor

Adds type class instances for immutable.Seq. This is based off of the implementations of the List and Vector typeclasses.

Closes #1222

Joseph Moniz and others added 5 commits September 30, 2020 12:23
Adds type class instances for `immutable.Seq`. This is based off of
the implementations of the `List` and `Vector` typeclasses.
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020

Codecov Report

Merging #3620 into master will decrease coverage by 0.76%.
The diff coverage is 53.94%.

@@            Coverage Diff             @@
##           master    #3620      +/-   ##
==========================================
- Coverage   91.08%   90.31%   -0.77%     
==========================================
  Files         385      391       +6     
  Lines        8602     8869     +267     
  Branches      260      247      -13     
==========================================
+ Hits         7835     8010     +175     
- Misses        767      859      +92     

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for this PR, it's very thorough! :) I think we should future proof a little bit, based on my experiences in #3552

@@ -82,6 +82,7 @@ object FunctorFilter extends ScalaVersionSpecificTraverseFilterInstances {
implicit def catsTraverseFilterForOption: TraverseFilter[Option] =
cats.instances.option.catsStdTraverseFilterForOption
implicit def catsTraverseFilterForList: TraverseFilter[List] = cats.instances.list.catsStdTraverseFilterForList
implicit def catsTraverseFilterForSeq: TraverseFilter[Seq] = cats.instances.seq.catsStdTraverseFilterForSeq
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to move this instance down since I believe dotty will have a hard time disambiguating between this and the vector and list instances.

@@ -113,6 +113,8 @@ object Invariant extends ScalaVersionSpecificInvariantInstances with InvariantIn
cats.instances.option.catsStdInstancesForOption
implicit def catsInstancesForList: Monad[List] with Alternative[List] with CoflatMap[List] =
cats.instances.list.catsStdInstancesForList
implicit def catsInstancesForSeq: Monad[Seq] with Alternative[Seq] with CoflatMap[Seq] =
Copy link
Member

Choose a reason for hiding this comment

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

Same goes for this

@@ -122,6 +122,7 @@ object SemigroupK extends ScalaVersionSpecificMonoidKInstances {

implicit def catsMonoidKForOption: MonoidK[Option] = cats.instances.option.catsStdInstancesForOption
implicit def catsMonoidKForList: MonoidK[List] = cats.instances.list.catsStdInstancesForList
implicit def catsMonoidKForSeq: MonoidK[Seq] = cats.instances.seq.catsStdInstancesForSeq
Copy link
Member

Choose a reason for hiding this comment

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

And this one

@PipelineBaron
Copy link
Contributor Author

Thanks for the review @LukaJCB!

I went ahead and lowered the priority of the immutable.Seq instances you called out. Hopefully they wont cause any problems for dotty going forward now.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

This is quite nice!

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.

No typeclasses for Seq
5 participants