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

OptionT implements MonadCombine, but doesn't meet the laws #693

Closed
mikejcurry opened this issue Nov 21, 2015 · 3 comments
Closed

OptionT implements MonadCombine, but doesn't meet the laws #693

mikejcurry opened this issue Nov 21, 2015 · 3 comments

Comments

@mikejcurry
Copy link
Contributor

Whilst looking at OptionT today and adding some tests in #692, I noticed that the OptionT has a MonadCombine instance, plus implementations for empty and combine.

The tests exercise MonadCombineTests.monad instead of MonadCombineTests.monadCombine, which means that the implementations of empty and combine are not tested against the laws (or anything).

I briefly looked at increasing the test to monadCombine, but the current implementation of OptionT does not pass the laws, at least not for OptionT[List, Int].

I noticed that #687 proposes to remove, at least temporarily, the MonadCombine for XorT - if a similar approach is warranted here I can take a look. By similiar in this instance, I mean that it might make sense to reduce the implementation to Monad until the laws pass.

@mikejcurry mikejcurry changed the title OptionT implements MonadCombine, but doesn't meet the laws OptionT implements MonadCombine, but doesn't meet the laws Nov 21, 2015
@ceedubs
Copy link
Contributor

ceedubs commented Nov 22, 2015

Good catch! Thank you for bringing this to attention.

I think that the right approach would probably be to:

  1. Remove this instance
  2. Make sure we have an issue for separating the weak and strong laws suggested by those comments in the XorT code
  3. Create new issues for adding back in the XorT and OptionT instances once the laws work has been finished.

How does that sound?

@mikejcurry
Copy link
Contributor Author

That sounds sensible. I can put together a PR(after #692 - as both PRs will touch OptionTTests) and create issues for tracking.

@ceedubs
Copy link
Contributor

ceedubs commented May 14, 2016

I'm going to go ahead and close this out since it was resolved with #698 and #695 tracks the follow-up.

@ceedubs ceedubs closed this as completed May 14, 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

No branches or pull requests

2 participants