Add MonadError.ensure method

[julienrf]

This adds an ensure method to MonadError.

I have a couple of questions:

• I think it is not possible to implement it in ApplicativeError, is it?
• Is it worth adding the following law? ensure(fa)(_ => true)(error) <-> fa
• Also, see my comment in the diff.

[julienrf]

I noticed that the applicativeErrorSyntax method was defined for a F[_, _] (notice the kind), but I have type inference issues with it. So here I defined it for a F[_]. Am I wrong?

[ceedubs]

No, I think that you are right. Coincidentally this just came up in a gitter conversation.

[ceedubs]

Thanks, @julienrf! I like this.

I think it is not possible to implement it in ApplicativeError, is it?

Yeah, I think you are right.

Is it worth adding the following law? filterOrRaiseError(fa)(_ => true)(error) <-> fa

That's a good question. I think I might have gone overboard with laws on MonadError, which has resulted in a ridiculous number of implicit parameters. It may make more sense to test against the default implementation, since the law you mentioned is equivalent to monad right identity in this case.

This method is equivalent to ensure on Xor. What do you think about simplifying the name to ensure and making the argument order the same so that the function argument comes last (to make it prettier to wrap onto the next line)?

[julienrf]

This method is equivalent to ensure on Xor. What do you think about simplifying the name to ensure and making the argument order the same so that the function argument comes last (to make it prettier to wrap onto the next line)?

Oh, yes, I will ensure that. (I didn’t notice this method on Xor)

[ceedubs]

👍 lgtm

Thanks @julienrf!

[julienrf]

Actually there is a problem: the tests don’t test the ensure implementation in MonadError because it has been overriden by Xor’s instance.

Maybe I should roll my own Xor-like type, just for the tests…

[ceedubs]

@julienrf ah good catch. There's a MonadError instance for Future. Maybe you could test that one?

[julienrf]

Hi, I changed the tests to use MonadError[Option, Unit] instead.

[codecov-io]

Current coverage is 88.51%

Merging #1013 into master will decrease coverage by <.01%

1. 3 files (not in diff) in .../main/scala/cats/std were modified. more
2. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
3. 2 files (not in diff) in ...main/scala/cats/data were modified. more
   • Misses +3
   • Hits -5
4. 1 files (not in diff) in .../src/main/scala/cats were modified. more
5. File .../cats/data/Xor.scala was modified. more
   • Misses -3
   • Partials 0
   • Hits +6

@@             master      #1013   diff @@
==========================================
  Files           214        216     +2   
  Lines          2719       2724     +5   
  Methods        2655       2662     +7   
  Messages          0          0          
  Branches         59         57     -2   
==========================================
+ Hits           2407       2411     +4   
- Misses          312        313     +1   
  Partials          0          0          

Powered by Codecov. Last updated by 271c197...1752755

[ceedubs]

I don't understand why this is reporting a code coverage drop - it doesn't seem to me like it should result in one.

👍 thanks!

[non]

This is a small thing, but do you think it would be possible to reverse the order or the error and the predicate?

I feel like x.ensuring(_ > 10)(someError) reads better than x.ensuring(someError)(_ > 10).

[ceedubs]

@non I think that elsewhere (with ap being the exception), we've used functions as the last argument. I think that it tends to look better when the function is multiple lines. I'd be inclined to follow that convention unless we have a strong reason not to.

[non]

Fair enough. For whatever reason I read the call as "ensuring " and so seeing the error first confused me.

I wonder if something like this would be complimentary?

def errorIf(fa: F[A])(f: A => Option[E]): F[A] =
  flatMap(fa)(a => f(a) match {
    case None => pure(a)
    case Some(e) => raiseError(e)
  })

[julienrf]

This is a small thing, but do you think it would be possible to reverse the order or the error and the predicate?

I often have a predicate longer than the error value so for me it often reads better to have the error value first.

errorIf is interesting (we use something similar at work), but would it be OK for you to leave it for another PR?

[non]

Sure! 👍