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 some helper methods to MonadError #562

Merged
merged 5 commits into from
Oct 8, 2015

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Oct 5, 2015

You should make me write unit tests and ScalaDocs for these before you
merge. I wanted to open a PR before I forgot about this though.

You should make me write unit tests and ScalaDocs for these before you
merge. I wanted to open a PR before I forgot about this though.
@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 5, 2015

Also I should override some of these with optimized versions in some of the instances.

@adelbertc
Copy link
Contributor

Awesome possum

@codecov-io
Copy link

Current coverage is 76.00%

Merging #562 into master will increase coverage by +0.24% as of fced280

@@            master   #562   diff @@
=====================================
  Files          156    157     +1
  Stmts         2117   2155    +38
  Branches        68     68       
  Methods          0      0       
=====================================
+ Hit           1604   1638    +34
  Partial          0      0       
- Missed         513    517     +4

Review entire Coverage Diff as of fced280

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 6, 2015

I started to add some laws, but I went down a rabbit hole upon realizing that Scalacheck doesn't provide generators for partial functions.

@non
Copy link
Contributor

non commented Oct 6, 2015

You can always create a PF[A, B] from A => Option[B] right?

Arbitrary(arbitrary[A => Option[B]].map { f =>
  new PartialFunction[A, B] {
    def isDefinedAt(a: A): Boolean = f(a).isDefined
    def apply(a: A): B = f(a).get
  })

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 6, 2015

@non right. There's even a Function.unlift method for that. Hence the rabbit hole :P

These probably don't have the best names. I'm open to naming
suggestions.
@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 7, 2015

I added some MonadError laws that hit the methods I've added. Their names probably aren't the greatest. I'm open to naming suggestions.

I'm going to add some optimized overrides, and then this PR will be ready for review (but feel free to comment now).

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 7, 2015

As I was documenting some of the MonadError methods, I convinced myself that handleError should become handleErrorWith and there should be a new handleError method that takes f: E => A instead of f: E => F[A]. This makes it line up with recover and recoverWith (from this PR on MonadError, but currently existing on Xor, XorT, and scala.concurrent.Future). So now I'm down a bit of another rabbit hole. Do people like the idea of that? It doesn't need to be an abstract method, because it can be implemented in terms of handleErrorWith and pure.

@non
Copy link
Contributor

non commented Oct 7, 2015

That sounds good to me. I say go for it! We have time to change it if it doesn't end up being good in practice.

Many of these "laws" are basically tests for default implementations.
I'm not sure whether or not they should exist in their current form.
This is something that has been talked about in the past, but I don't
think we really reached a verdict on how to go about this.
@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 8, 2015

Okay I pushed some more changes.

Many of these "laws" are basically tests for default implementations. I'm not sure whether or not they should exist in their current form. This is something that has been talked about in the past, but I don't think we really reached a verdict on how to go about this. I figured it probably doesn't hurt much to add them for now, and we can always rip them out later.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 8, 2015

Okay I pushed some more changes.

That didn't make it very clear, but I think this is ready to review, and I don't have any pending changes.

@adelbertc
Copy link
Contributor

Woaaa awesome 👍 from me

@non
Copy link
Contributor

non commented Oct 8, 2015

I 100% support laws for testing default implementations (I think they are great examples of coherence that a user can expect), so +1 from me.

@non
Copy link
Contributor

non commented Oct 8, 2015

Looks like there is a Scaladoc error here, but if that is cleaned up 👍 from me.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 8, 2015

Build is green again. I'm going to go ahead and merge.

ceedubs added a commit that referenced this pull request Oct 8, 2015
Add some helper methods to MonadError
@ceedubs ceedubs merged commit 37467e4 into typelevel:master Oct 8, 2015
@ceedubs ceedubs deleted the monad-error-helpers branch October 8, 2015 01:15
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.

4 participants