Add ensureWith to Validated and Either (#1550) #1612

LukaJCB · 2017-04-18T15:00:01Z

Resolves #1550. Also looking for feedback on naming here, or maybe I should be adding an additional test apart from the doc test. Would be much appreciated :)

kailuowang · 2017-04-18T16:20:30Z

I think additional tests would be nice for these logic.

codecov-io · 2017-04-19T22:17:37Z

Codecov Report

Merging #1612 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1612      +/-   ##
==========================================
+ Coverage   93.38%   93.43%   +0.04%     
==========================================
  Files         241      241              
  Lines        4054     4068      +14     
  Branches      134      138       +4     
==========================================
+ Hits         3786     3801      +15     
+ Misses        268      267       -1

Impacted Files	Coverage Δ
laws/src/main/scala/cats/laws/MonadErrorLaws.scala	`100% <100%> (ø)`	⬆️
...n/scala/cats/laws/discipline/MonadErrorTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadError.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/either.scala	`100% <100%> (+2.38%)`	⬆️
core/src/main/scala/cats/MonadError.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/syntax/either.scala	`99.13% <100%> (+0.02%)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/EitherT.scala	`96.52% <100%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb949b8...a76d857. Read the comment docs.

ceedubs · 2017-04-25T12:31:48Z

It looks like ScalaDoc isn't happy about the $s in your message, because it's trying to interpret it as a ScalaDoc variable. You can change it to \$s or you can use traditional string concatenation instead of string interpolation to work around it.

LukaJCB · 2017-04-27T11:29:36Z

Thank you @ceedubs! CI seems to be happy now :)

peterneyens · 2017-04-27T14:21:07Z

tests/src/test/scala/cats/tests/ValidatedTests.scala

+    }
+  }
+
+  test("ensureWith should fail if predicate not satisfied") {


Should we add this test to EitherTests as well?

ceedubs · 2017-04-28T13:03:28Z

Thanks @LukaJCB! This LGTM. The only thing that I'm not sure about is the name ensureWith. We have recoverWith defined in ApplicativeError, and the With there means something a little different. Having said that, I don't have a better name in mind and ultimately I'm okay with ensureWith if nobody has a better proposal.

It doesn't necessarily need to be done in this PR, but I think that ensureWith could also be added as a derived method alongside ensure in the MonadError type class.

LukaJCB · 2017-04-28T13:09:40Z

Thanks for your feedback! :)
+1 for keeping it open to see if there's a better name! I can create another PR for the same method in MonadError soon! :)

kailuowang · 2017-04-28T14:23:51Z

Given that we are close to 1.0.0-MF, I kind of prefer having the addition to MonadError and these in a single PR. I noticed that the MonadError instance for EitherT did not override the ensure method. So there are probably some optimizations can be done.
As for the name, I am probably the last person to give a better proposal but how about ensureOr? It's not more intuitive than ensureWith but at least it does not conflict with recoverWith.

kailuowang · 2017-05-05T01:46:43Z

This is going to be really useful. I constantly need this feature.
I would like to make the following suggestion.

add the two new laws to MonadErrorLaws something like

def monadErrorEnsureConsistency[A](fa: F[A], e: E, p: A => Boolean): IsEq[F[A]] =
    F.ensure(fa)(e)(p) <-> F.flatMap(fa)(a => if (p(a)) F.pure(a) else F.raiseError(e))

def monadErrorEnsureOrConsistency[A](fa: F[A], e: A => E, p: A => Boolean): IsEq[F[A]] =
    F.ensure(fa)(e)(p) <-> F.flatMap(fa)(a => if (p(a)) F.pure(a) else F.raiseError(e(a)))

override the ensureOr method in the MonadError instances for Either and EitherT , override the ensure method in the MonadError instance of EitherT, just like here
remove the ensure and ensureOr tests in EitherTests and EitherTTests, since they will be covered by the law tests.

LukaJCB · 2017-05-06T11:39:12Z

So implemented the changes, but removing the tests seems to have reduced the coverage, will investigate soon :D

kailuowang · 2017-05-06T11:41:28Z

The newly added laws need to go into the tests in the discipline package.

LukaJCB · 2017-05-06T13:10:25Z

Oops, yeah, that should've been obvious to me, fixed!

ceedubs

Thanks @LukaJCB! 👍 LGTM

ceedubs · 2017-05-13T18:54:11Z

core/src/main/scala/cats/syntax/either.scala

@@ -63,6 +63,11 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
    case Right(b) => if (f(b)) eab else Left(onFailure)
  }

+  def ensureOr[AA >: A](onFailure: B => AA)(f: B => Boolean): Either[AA, B] = eab match {


It looks like this is following a precedent, but I find myself wondering if this Either syntax should really be providing syntax for methods that should already be available via type class syntax (in this case MonadError). Let's table that for a separate issue though.

kailuowang · 2017-05-15T15:58:43Z

thanks so much @LukaJCB , sorry about the delay. LGTM. 👍
@peterneyens any other thoughts/feedback?

peterneyens · 2017-05-15T16:21:33Z

There seem to be some merge conflicts, otherwise this looks OK 👍 , thanks @LukaJCB !

I wonder if we maybe should implement ensure in terms of ensureOr that way just overriding ensureOr gives you a more efficient ensure as well.

kailuowang · 2017-05-15T20:56:53Z

I took the liberty of resolving the conflict using github's merging too. Will merge if build green.

Add ensureWith (typelevel#1550)

66031e4

Add test cases for ensureWith

a3e9fdd

Remove String interpolation from Doc test to fix clash with ScalaDoc

13e7268

peterneyens reviewed Apr 27, 2017

View reviewed changes

Add additional test for Either.ensureWith

f81e872

Luka Jacobowitz added 2 commits May 4, 2017 23:45

Rename ensureWith to ensureOr

f16a3e7

Add ensureWith to MonadError

f897168

kailuowang added the Binary Breaking label May 5, 2017

kailuowang added this to the 1.0.0-MF milestone May 5, 2017

Luka Jacobowitz and others added 4 commits May 5, 2017 17:23

Add ensure and ensureOr laws to MonadError

20f5e27

Remove ensure and ensureOr Tests

42c93b2

Override ensure and ensureOr in instances of Either and EitherT

46e982a

Merge branch 'master' into add-ensure-with

790f7f5

Add MonadError Laws to tests

6ef5fa5

ceedubs approved these changes May 13, 2017

View reviewed changes

Merge branch 'master' into add-ensure-with

a76d857

kailuowang merged commit 465b13a into typelevel:master May 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ensureWith to Validated and Either (#1550) #1612

Add ensureWith to Validated and Either (#1550) #1612

LukaJCB commented Apr 18, 2017

kailuowang commented Apr 18, 2017

codecov-io commented Apr 19, 2017 •

edited

Loading

ceedubs commented Apr 25, 2017

LukaJCB commented Apr 27, 2017

peterneyens Apr 27, 2017

LukaJCB Apr 28, 2017

ceedubs commented Apr 28, 2017

LukaJCB commented Apr 28, 2017

kailuowang commented Apr 28, 2017

kailuowang commented May 5, 2017

LukaJCB commented May 6, 2017

kailuowang commented May 6, 2017

LukaJCB commented May 6, 2017

ceedubs left a comment

ceedubs May 13, 2017

kailuowang commented May 15, 2017

peterneyens commented May 15, 2017

kailuowang commented May 15, 2017

Add ensureWith to Validated and Either (#1550) #1612

Add ensureWith to Validated and Either (#1550) #1612

Conversation

LukaJCB commented Apr 18, 2017

kailuowang commented Apr 18, 2017

codecov-io commented Apr 19, 2017 • edited Loading

Codecov Report

ceedubs commented Apr 25, 2017

LukaJCB commented Apr 27, 2017

peterneyens Apr 27, 2017

Choose a reason for hiding this comment

LukaJCB Apr 28, 2017

Choose a reason for hiding this comment

ceedubs commented Apr 28, 2017

LukaJCB commented Apr 28, 2017

kailuowang commented Apr 28, 2017

kailuowang commented May 5, 2017

LukaJCB commented May 6, 2017

kailuowang commented May 6, 2017

LukaJCB commented May 6, 2017

ceedubs left a comment

Choose a reason for hiding this comment

ceedubs May 13, 2017

Choose a reason for hiding this comment

kailuowang commented May 15, 2017

peterneyens commented May 15, 2017

kailuowang commented May 15, 2017

codecov-io commented Apr 19, 2017 •

edited

Loading