Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add setMonoid #497

Merged
merged 3 commits into from
Sep 11, 2015
Merged

add setMonoid #497

merged 3 commits into from
Sep 11, 2015

Conversation

drostron
Copy link
Contributor

@codecov-io
Copy link

Current coverage is 63.82%

Merging #497 into master will increase coverage by +0.92% as of d74e9fc

@@            master    #497   diff @@
======================================
  Files          154     155     +1
  Stmts         2359    2366     +7
  Branches        66      66       
  Methods          0       0       
======================================
+ Hit           1484    1510    +26
  Partial          0       0       
+ Missed         875     856    -19

Review entire Coverage Diff as of d74e9fc

Powered by Codecov. Updated on successful CI builds.

@non
Copy link
Contributor

non commented Aug 30, 2015

👍 Thanks!

@ceedubs
Copy link
Contributor

ceedubs commented Aug 30, 2015

It will probably require adding a questionable Eq instance for sets, but could we test this against the monoid laws?

@drostron
Copy link
Contributor Author

Sure. I need to familiarize with law checking a bit more first (been itching to do so for awhile so this is a welcomed opportunity). The next few days are filled, but should have some time after that.

@drostron
Copy link
Contributor Author

drostron commented Sep 9, 2015

Pushed a commit adding law checking for setMonoidInstance. @ceedubs, I'm curious if this PR introduces a questionable Eq instance for sets. If so I'm interested to learn more.

@ceedubs
Copy link
Contributor

ceedubs commented Sep 9, 2015

👍 looks good to me.

I was surprised to see that Eq instance for sets was in scope. It looks like algebra has provided one for quite a while (not introduced with this PR). I suppose it makes sense to have it if you are working with Set, since it's based on universal equality anyway.

@drostron drostron changed the title add setMonoidInstance add setMonoid Sep 10, 2015
@ceedubs
Copy link
Contributor

ceedubs commented Sep 11, 2015

The only thing added since @non gave a 👍 is law-checking which I can't imagine he would be opposed to :). I'm going to merge this. Thanks!

ceedubs added a commit that referenced this pull request Sep 11, 2015
@ceedubs ceedubs merged commit 2d5c6e1 into typelevel:master Sep 11, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants