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 map method to OneAnd. Fix for #885 #886

Merged
merged 5 commits into from
Feb 17, 2016
Merged

Add map method to OneAnd. Fix for #885 #886

merged 5 commits into from
Feb 17, 2016

Conversation

pvillega
Copy link

OneAnd was missing an instance of map. This commit tries to solve this.

Add map method to OneAnd. Fix for typelevel#885
3679f00 
OneAnd was missing an instance of map. This commit tries to solve this.
@adelbertc
Copy link
Contributor

👍 - could you also replace the existing map definitions in the type class instances with the new map?

@pvillega
Copy link
Author

will do :)

@codecov-io
Copy link

Current coverage is 89.05%

Merging #886 into master will decrease coverage by -0.29% as of 0f0402f

@@            master    #886   diff @@
======================================
  Files          169     174     +5
  Stmts         2337    2384    +47
  Branches        75      76     +1
  Methods          0       0       
======================================
+ Hit           2088    2123    +35
  Partial          0       0       
- Missed         249     261    +12

Review entire Coverage Diff as of 0f0402f

Powered by Codecov. Updated on successful CI builds.

Replace the existing map definitions in the type class instances wit…
b358cd2 
…h the new map

 CoMonad required a explicit Functor[List], there may be a better alternative
@pvillega
Copy link
Author

Done! I'm not sure about the val functorList: Functor[List] I had to define for the CoMonad implicit. I couldn't see an existing alternative but there may be one, if so please let me know and I'll correct it tomorrow (UK time).

@adelbertc
Copy link
Contributor

You probably added it because the type checker couldn't find a Functor[List] in implicit scope - you can get it from import cats.std.list._.

Replace Functor by existing Cats definition of Functor
2244862 
Import at trait level due to conflict with Semigroup definitions
adelbertc
adelbertc reviewed Feb 16, 2016
View reviewed changes
core/src/main/scala/cats/data/OneAnd.scala
@@ -131,9 +137,10 @@ private[data] sealed trait OneAndInstances extends OneAndLowPriority2 {
}

trait OneAndLowPriority0 {
import cats.std.list._
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: would prefer this to be moved to the top

@adelbertc
Copy link
Contributor

One minor comment, but 👍 on Travis green

@adelbertc
Copy link
Contributor

Hm seems moving it to the top does funky things - anyone know why this is the case? Either way, 👍 from me

Move import to the top and fix compilation issue
579fb2f 
Thanks @julien-truffaut for his help fixing this compilation error!
@pvillega
Copy link
Author

Fixed compilation issue when moving import to top, thanks to @julien-truffaut for his support (he was the one fixing it in fact, I'm just stealing the contribution :P)

ceedubs
ceedubs reviewed Feb 17, 2016
View reviewed changes
core/src/main/scala/cats/data/OneAnd.scala
@@ -104,7 +111,7 @@ private[data] sealed trait OneAndInstances extends OneAndLowPriority2 {
}

implicit def oneAndSemigroup[F[_]: MonadCombine, A]: Semigroup[OneAnd[F, A]] =
oneAndSemigroupK.algebra
oneAndSemigroupK(MonadCombine[F]).algebra
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is nitpicky, but could you please change this to oneAndSemigroupK[F].algebra? That will save a MonadCombine.apply call.

Copy link
Contributor

Choose a reason for hiding this comment

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

for some reason the compiler get confused with oneAndSemigroupK[F] after adding import cats.std.list._ on the top of the file. It says that there is an ambiguity between MonadCombine[List] and MonadCombine[F] ...

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I misread your comment @ceedubs, you were right oneAndSemigroupK[F] is enough to disambiguate.

@julien-truffaut
Copy link
Contributor

👍 when the build is green

julien-truffaut added a commit that referenced this pull request Feb 17, 2016
@julien-truffaut
Merge pull request #886 from pvillega/master
8c35756 
Add map method to OneAnd. Fix for #885
@julien-truffaut julien-truffaut merged commit 8c35756 into typelevel:master Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@pvillega @adelbertc @codecov-io @julien-truffaut @ceedubs