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 monoid syntax #1130

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Add monoid syntax #1130

merged 2 commits into from
Jun 16, 2016

Conversation

hamishdickson
Copy link
Contributor

This resolves #1124

There are a couple of questions

  • in Group and SemiGroup the comment // TODO: use simulacrum instances eventually has been made. I have copied this so all 3 can be changed at the same time. Is that the right thing to do here? I'm happy to use simulacrum if not
  • in implementing isEmpty I originally attempted to use Ops.binOp but couldn't get it to work with my implicit Eq. Should I be using that here?

Sorry they are such basic questions, this is my first attempt at contributing to Cats and I'm trying to find my feet!

@codecov-io
Copy link

Current coverage is 88.90%

Merging #1130 into master will decrease coverage by 0.05%

@@             master      #1130   diff @@
==========================================
  Files           227        228     +1   
  Lines          2991       2993     +2   
  Methods        2940       2942     +2   
  Messages          0          0          
  Branches         48         48          
==========================================
  Hits           2661       2661          
- Misses          330        332     +2   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 3c8f03c...6ad6b72

@ceedubs
Copy link
Contributor

ceedubs commented Jun 15, 2016

@hamishdickson thanks for working on this!

in Group and SemiGroup the comment // TODO: use simulacrum instances eventually has been made. I have copied this so all 3 can be changed at the same time. Is that the right thing to do here? I'm happy to use simulacrum if not

I think that you can remove the comments. We've decided not to use simulacrum within cats-kernel for the time being.

in implementing isEmpty I originally attempted to use Ops.binOp but couldn't get it to work with my implicit Eq. Should I be using that here?

I don't understand machinist very well. I thought that you'd be able to do something like def isEmpty(implicit ev: Eq[A]): Boolean = macro Ops.unopWithEv[Eq[A], A], and that compiles, but then when you try to use it elsewhere, you get the error Cannot extract subject of operator (tree = cats.implicits.catsSyntaxMonoid[Int](1)(cats.implicits.intGroup)). Paging @non!

@ceedubs
Copy link
Contributor

ceedubs commented Jun 15, 2016

PS @hamishdickson I had forgotten about the usage of machinist, and this issue wasn't as much "low-hanging-fruit" as I was thinking it was. Sorry about that!

@hamishdickson
Copy link
Contributor Author

@ceedubs my pleasure and no need to say sorry! I found it really interesting trying to get machinist to work (I also tried unopWithEv) but didn't want to spend too long on it unless it was obviously the wrong thing to do

If @non doesn't know off the top of his head, I will have a deeper look into machinist 😄

Another noob question (sorry) - should I be updating any docs with this change?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 15, 2016

Another noob question (sorry) - should I be updating any docs with this change?

There's definitely no need to apologize. If you feel inclined, you could add an example of using the isEmpty syntax on the monoid tutorial.

@non
Copy link
Contributor

non commented Jun 16, 2016

@ceedubs so there are two evidence parameters here (Monoid[A] and Eq[A]) so in this case I think you'd need to use unopWithEv2. The documentation for Machinist needs to be a lot better before I can expect someone else to figure that one out!

EDIT: This still seems to have a problem. I'd say let's merge what you have and I'll look into Machinist.

👍

@ceedubs ceedubs merged commit 400c24d into typelevel:master Jun 16, 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.

Add Monoid syntax
4 participants