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.

Sign up for GitHub

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

Ported IO#ensuring (and renamed to avoid mass confusion) #1662

Closed
wants to merge 4 commits into from
Closed

Ported IO#ensuring (and renamed to avoid mass confusion) #1662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b86f987
Ported IO#ensuring (and renamed to avoid mass confusion)
djspiewak May 11, 2017
4e5ffcc
Merge branch 'master' into feature/ensuring
djspiewak May 25, 2017
340157b
Added tests for MonadError#guarantee
djspiewak Jun 4, 2017
dac34b9
Merge branch 'master' into feature/ensuring
djspiewak Jun 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions core/src/main/scala/cats/MonadError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,23 @@ trait MonadError[F[_], E] extends ApplicativeError[F, E] with Monad[F] {
def ensureOr[A](fa: F[A])(error: A => E)(predicate: A => Boolean): F[A] =
flatMap(fa)(a => if (predicate(a)) pure(a) else raiseError(error(a)))

/**
* Sequences the specified finalizer ensuring evaluation regardless of
* whether or not the target `F[A]` raises an error.
*
* If `raiseError` is analogous to `throw` and `handleErrorWith` is analogous to
* `catch`, then `guarantee` is analogous to `finally`. JVM exception semantics
* are mirrored with respect to error raised within the finalizer (i.e. errors
* raised within the finalizer will shade any errors raised by the primary action).
*
* @see [[raiseError]]
* @see [[handleErrorWith]]
*/
def guarantee[A](fa: F[A], finalizer: F[Unit]): F[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test of some kind to this?

I mean, even adding a reimplementation of this code to the laws for MonadError along with an example exercising it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on adding to law. maybe also add quick doctest as an example and to provide coverage for the syntax code.

Copy link
Member Author

@djspiewak djspiewak May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A law would be useless, since it would over-constrain implementations. The only useful overrides of guarantee would be for implementations which have it as a structural primitive, but in that case guarantee would not be equivalent to flatMap composed with attempt. So it would be a very round-about way of making the function final and running a ton of redundant tests.

The right thing to do is add a test, but unlike MonadTest, MonadErrorTest doesn't currently exist. I'm all in favor of adding it, but this falls into the category of things that are somewhat harder to work on (since I have to figure out how all that stuff works). Incoming sometime in the next few days, hopefully.

Copy link
Contributor

@kailuowang kailuowang May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the MonadTests, it's basically just a vanilla scalatests suite. It should be trivial to create a MonadErrorTest class and just write regular scalatests tests for guarantee in it.

Update: actually there is already a MonadErrorTest it's called MonadErrorSuite, we should probably rename it.

Copy link
Contributor

@edmundnoble edmundnoble May 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djspiewak If guarantee is a structural primitive, it must still behave as flatMap composed with attempt, no? I don't see how anything otherwise could be the case. What is the difference between a law and a test in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a situation where a datatype adds extra guarantees to guarantee, meaning that it would have distinct semantics from flatMap composed with attempt. I certainly can't prove that this is impossible, so I wouldn't want to rule it out completely with a law.

In general, I feel like concrete convenience functions (like this one) should be tested with tests, not laws, especially when their behavior follows trivially from other laws.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to see tests here specified in terms of the "bracket problem" (see http://hackage.haskell.org/package/layers-0.1/docs/Documentation-Layers-Overview.html#g:9 and the next couple sections).

I am not convinced this belongs on MonadError. As we've discussed previously, a monad layer being able to return an error has nothing to do with the rest of the effects in the stack being able to halt the computation, which is what guarantee exists to guarantee semantics for.

flatMap(attempt(fa)) { e =>
flatMap(finalizer)(_ => e.fold(raiseError, pure))
}
}
}

object MonadError {
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/scala/cats/syntax/monadError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@ trait MonadErrorSyntax {
}

final class MonadErrorOps[F[_], E, A](val fa: F[A]) extends AnyVal {

def ensure(error: => E)(predicate: A => Boolean)(implicit F: MonadError[F, E]): F[A] =
F.ensure(fa)(error)(predicate)

def ensureOr(error: A => E)(predicate: A => Boolean)(implicit F: MonadError[F, E]): F[A] =
F.ensureOr(fa)(error)(predicate)

def guarantee(finalizer: F[Unit])(implicit F: MonadError[F, E]): F[A] =
F.guarantee(fa, finalizer)
}
32 changes: 31 additions & 1 deletion ...st/scala/cats/tests/MonadErrorSuite.scala → ...est/scala/cats/tests/MonadErrorTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package cats
package tests

class MonadErrorSuite extends CatsSuite {
import data.StateT

import scala.language.postfixOps

class MonadErrorTest extends CatsSuite {

val successful: Option[Int] = 42.some
val failed: Option[Int] = None
Expand Down Expand Up @@ -32,5 +36,31 @@ class MonadErrorSuite extends CatsSuite {
failed.ensureOr(_ => ())(_ => true) should === (failed)
}

{
import StateTTests._

type Test[A] = StateT[Either[Boolean, ?], Int, A]

val successful: Test[String] = StateT.modify[Either[Boolean, ?], Int](1 +).map(_ => "foo")
val failed1: Test[String] = StateT.lift(Left(true))
val failed2: Test[String] = StateT.lift(Left(false))
val finalizer: Test[Unit] = StateT.modify[Either[Boolean, ?], Int](10 *)

test("guarantee returns successful") {
successful.guarantee(finalizer) should === (successful.flatMap(a => finalizer.map(_ => a)))
}

test("guarantee runs finalizer on fail") {
val expected: Test[String] = for {
_ <- StateT.modify[Either[Boolean, ?], Int](10 *)
_ <- failed1
} yield "foo"

failed1.guarantee(finalizer) should === (expected)
}

test("guarantee returns inner errors on double failure") {
failed1.guarantee(failed2 >> ().pure[Test]) should === (failed2)
}
}
}