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

Add ApplySemigroup and ApplicativeMonoid #1128

Merged
merged 2 commits into from
Jun 15, 2016

Conversation

johnynek
Copy link
Contributor

these are always lawful. The generalization to Group may not be (see Try, Failure has no inverse) or Semilattice (Apply.map2(a, a)(_._1) might not be idempotent, but if it is, we can add Band and Semilattice.

@non
Copy link
Contributor

non commented Jun 14, 2016

This looks great!

We should run the law-checker to verify these. I can try to dig up a snippet for you but it should be pretty easy to get it working.

@johnynek
Copy link
Contributor Author

@non I think we are testing try at least, so I think they are tested that way.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 14, 2016

👍 looks good to me, and the project code coverage has gone up, so they do appear to be tested.

@johnynek these are public classes so they will become a part of the cats API. That seems reasonable to me as other people could potentially benefit from these, but I just wanted to confirm that we are okay with that and the compatibility constraints that it adds.

@johnynek
Copy link
Contributor Author

I think we want them public so they can be reused indeed.

@johnynek
Copy link
Contributor Author

actually, I'm not sure they need to be abstract at all in that case.

That said, since they are so small, maybe we should hide them. Most users can easily reimplement. Instead we could put:

object Apply {
  def semigroup[F[_]: Apply, A: Semigroup]: Semigroup[F[A]] = ...
}

object Applicative {
  def monoid[F[_]: Apply, A: Monoid]: Monoid[F[A]] = ...
}

We may or may not make those implicit for someone to import.

Then the only people that miss out are those that know they have some other special structure (like they know they can make a Group or a Semilattice), but again, it is so little code to share, maybe no big deal.

Preferences?

@non
Copy link
Contributor

non commented Jun 14, 2016

I'd lean toward private. As you say, reimplementation is not hard.

@non
Copy link
Contributor

non commented Jun 14, 2016

Oh, right, I missed the fact that the Try instances were using them. 👍 from me (modulo making things private).

@codecov-io
Copy link

codecov-io commented Jun 14, 2016

Current coverage is 88.96%

Merging #1128 into master will not change coverage

@@             master      #1128   diff @@
==========================================
  Files           227        227          
  Lines          2991       2991          
  Methods        2937       2940     +3   
  Messages          0          0          
  Branches         51         48     -3   
==========================================
  Hits           2661       2661          
  Misses          330        330          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 763592f...e651f5b

@non non merged commit 3c8f03c into typelevel:master Jun 15, 2016
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.

5 participants