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

Adding cond and filterOrElse #968

Merged
merged 3 commits into from Jun 17, 2019

Conversation

Projects
None yet
4 participants
@blast-hardcheese
Copy link
Contributor

commented Jun 5, 2019

I'd like to be able to conveniently convert an A into an E, providing either a static E or converting the A into an E.

  • Note: cond could alternately be called filter/filterOr. I have not used ZIO enough to have a conception of the idiomatic naming scheme.

Inspiration:

cond(A => Boolean, A => E1)

filterOrElse(A => Boolean, => E1)

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:adding-cond-and-filterOrElse branch from cc2991e to 62681fd Jun 5, 2019

@dcsobral

This comment has been minimized.

Copy link

commented Jun 5, 2019

I think it would be preferable to take a PartialFunction. Like this:

  def rejectIf[R, E1, E2 <: E1, A](self: ZIO[R, E2, A])(pf: PartialFunction[A, E1]): ZIO[R, E1, A] = self.flatMap {
    case v if pf.isDefinedAt(v) => ZIO.fail(pf(v))
    case _ => self
  }
@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@dcsobral Good catch! Updating...

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Oh, actually, I didn't see the PartialFunction, I was more focused on flatMap(_ => self) instead of ZIO.success(...). I don't like PartialFunction here, as:

<console>:32: error: type mismatch;
 found   : Int => Long
 required: PartialFunction[Int,Long]
       foo(_.toLong)
             ^
@dcsobral

This comment has been minimized.

Copy link

commented Jun 5, 2019

Using a partial function you get both methods, and with nice syntax.

def testCond = {
    val goodCase = unsafeRun(
      ZIO.succeed(0).rejectIf{ case a if a != 0 => s"$a was not 0"}.sandbox.either
    ) must_=== Right(0)

    val badCase = unsafeRun(
      ZIO.succeed(5).rejectIf{ case a if a != 0 => s"$a was not 0"}.sandbox.either
    ).left.map(_.failureOrCause) must_=== Left(Left("5 was not 0"))

    goodCase and badCase
  }

  def testFilterOrElse = { // Note I inverted 0 and 5, to make for a simpler case condition
    val goodCase = unsafeRun(
      ZIO.succeed(5).rejectIf{ case 0 => "Predicate failed!"}.sandbox.either
    ) must_=== Right(5)

    val badCase = unsafeRun(
      ZIO.succeed(0).rejectIf{ case 0 => "Predicate failed!"}.sandbox.either
    ).left.map(_.failureOrCause) must_=== Left(Left("Predicate failed!"))

    goodCase and badCase
  }
@dcsobral

This comment has been minimized.

Copy link

commented Jun 5, 2019

It is true that you can't trivially convert a function into a partial function, but I still think it's better because a partial function is exactly what this is. And cond is just a pf that ignores the input parameter.

I'm not a fan of "cond" as a name. It's too generic a name. Not a fan of "filterOrElse" -- it is better, but orElse goes from error to no error, so filterOrElse doing the opposite might be unintuitive. That's why I went with rejectIf.

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@dcsobral I'm onboard with rejectIf, though that would be A => Boolean, => E1. A similar name would need to exist to describe A => Boolean, A => E1.

I don't consider { case a => f(a) } an acceptable answer for the base case, though PartialFunction[A, E1] is exactly what I'm trying to do. rejectWhen may be an acceptable answer as well, but rejecting the decomposed form of cond(_.foo) _ is undesirable, imo.

@NeQuissimus NeQuissimus force-pushed the zio:master branch 2 times, most recently from 898f033 to 3cf511d Jun 9, 2019

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

The generalization of this:

def cond(A => Boolean, A => E1)

is this:

trait ZIO[R, E, A] {
   def whenM(p: A => Boolean)(f: A => ZIO[R, E, A]): ZIO[R, E, A]

   def whenNotM(p: A => Boolean)(f: A => ZIO[R, E, A]): ZIO[R, E, A]
}

which also suggests variants:

trait ZIO[R, E, A] {
   def whenM_(p: A => Boolean)(f: ZIO[R, E, A]): ZIO[R, E, A]

   def whenNotM_(p: A => Boolean)(f: ZIO[R, E, A]): ZIO[R, E, A]
}

One could further specialize, but they are already relatively easy, e.g.:

foo.whenM_(_ < 0)(ZIO.fail("Must be positive"!))

Note there is existing ZIO precedent for the name when as well as the underscore for "unused" parameter variants.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

@dcsobral

I like this combinator. I would tweak it a bit (ignoring variance):

sealed trait ZIO[R, E, A] {
 def collect[B](e: E)(pf: PartialFunction[A, B]): ZIO[R, E, B] = ???
 def collectM[B](e: E)(pf: PartialFunction[A, ZIO[R, E, B]]): ZIO[R, E, B] = ???
 def reject(pf: PartialFunction[A, E]): ZIO[R, E, A] = ???
 def rejectM(pf: PartialFunction[A, ZIO[R, E, E]]): ZIO[R, E, A] = ???
}

I'd definitely take these, although I think of them as separate from the when* combinators.

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

So, to clarify, I should update this PR to provide definitions for:

trait ZIO[R, E, A] {
  def collect[E1 <: E, B](e: E1)(pf: PartialFunction[A, B]): ZIO[R, E1, B] = ???
  def collectM[E1 <: E, B](e: E1)(pf: PartialFunction[A, ZIO[R, E, B]]): ZIO[R, E1, B] = ???
  def reject[E1 <: E](pf: PartialFunction[A, E1]): ZIO[R, E1, A] = ???
  def rejectM[E1 <: E](pf: PartialFunction[A, ZIO[R, E1, E1]]): ZIO[R, E1, A] = ???

  def whenM[E1 <: E](A => Boolean)(f: A => ZIO[R, E1, A]): ZIO[R, E1, A]
  def whenNotM[E1 <: E](A => Boolean)(f: A => ZIO[R, E1, A]): ZIO[R, E1, A]
  def whenM_[E1 <: E](A => Boolean)(f: ZIO[R, E1, A]): ZIO[R, E1, A]
  def whenNotM_[E1 <: E](A => Boolean)(f: ZIO[R, E1, A]): ZIO[R, E1, A]
}

?

Also, should the constraint on whenM_ and whenNotM_'s f be ZIO[R, E1, Any] to explicitly indicate that it is not considered in the resultant A?

I'm most interested in the A => Boolean and A => E1 variants, as I hardly use PartialFunction and I'd rather not have to whenM(_.isFoo)(ZIO.fail(_)), as it permits sideways a => ZIO.pure(a) which seems like an overloaded flatMap. At least the method name is comprehensible, so I'd welcome the extra finger strain in order to get closer to my end goal.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Also, should the constraint on whenM_ and whenNotM_'s f be ZIO[R, E1, Any] to explicitly indicate that it is not considered in the resultant A?

It would be used, because the implementation will be something like:

... = self.flatMap(a => if (p(a)) f(a) else ZIO.succeed(a))

whenM(.isFoo)(ZIO.fail()), as it permits sideways a => ZIO.pure(a) which seems like an overloaded

It corresponds to changing the A value when a predicate holds.

so I'd welcome the extra finger strain in order to get closer to my end goal.

One layer of additional specialization would yield:

sealed trait ZIO[R, E, A] {
  def whenFail(p: A => Boolean)(f: A => E): ZIO[R, E, A]
  def whenFail_(p: A => Boolean)(f: A => E): ZIO[R, E, A]
}

And upon further reflection, when is used in slightly different context in ZIO / Ruby / etc., so I think different names are warranted, and your original stdlib suggestion seems fine.

So perhaps something more like:

def filterOrElse(p: A => Boolean)(f: A => ZIO[R, E, A]): ZIO[R, E, A]
def filterOrElse_(p: A => Boolean)(f: ZIO[R, E, A]): ZIO[R, E, A]

I dropped the M because the non-monadic versions cannot be written, so there are no variants. And I omitted the Not variants (filterNot) because I think they're uncommon.

So in total you'd have:

def filterOrElse(p: A => Boolean)(f: A => ZIO[R, E, A]): ZIO[R, E, A] = ???
def filterOrElse_(p: A => Boolean)(zio: => ZIO[R, E, A]): ZIO[R, E, A] = filterOrElse(p)(_ => zio)

def filterOrFail(p: A => Boolean)(e: => E): ZIO[R, E, A] = filterOrElse_(p)(ZIO.fail(e))

Separately, I do think collect and reject would be quite nice, though that could be a separate ticket / PR if desired (although I'd happily take them in this one 😄).

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Excellent. Thanks for the guidance!

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:adding-cond-and-filterOrElse branch 2 times, most recently from 8aba088 to bf3c863 Jun 10, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

OK. I've now implemented:

  • filterOrElse
  • filterOrElse
  • filterOrFail
  • collect
  • collectM
  • reject
  • rejectM

I just had one question, should rejectM actually be:

def rejectM[R1 <: R, E1 >: E, E2 <: E1](pf: PartialFunction[A, ZIO[R1, E2, E1]]): ZIO[R1, E1, A]

or some other variance? (Related to @dcsobral's comment from earlier)

Having PF[A, ZIO[R1, E1, E1]] seemed overly restrictive, but I'm not sure which way the variance makes the most sense.

There aren't many other methods that use both E1 and E2.

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:adding-cond-and-filterOrElse branch from bf3c863 to 4567a9e Jun 10, 2019

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Having PF[A, ZIO[R1, E1, E1]] seemed overly restrictive, but I'm not sure which way the variance makes the most sense.

Only E1 occurs in the output of the method, whereas your proposed E2 occurs in an input, in the ZIO effect which is already covariant in that type parameter.

Therefore, as far as I can tell, they should be equivalent, although you could test that to make sure Scalac isn't being weird. 😄

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

While messing around, I also discovered that

self.flatMap {
  case v if ! p(v) => ???
  case _ => self
}

executes self twice, which is very much not what we want to do in that case. Adding that as a test as well.

Proof:

scala> (console.putStrLn("hey") *> ZIO.succeed(5L)).filterOrFail(_ => true)(new Exception("fail")).provide(new console.Console.Live {}).unsafeRun
hey
hey
res4: Long = 5

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:adding-cond-and-filterOrElse branch from 74d96b7 to 0aa5221 Jun 11, 2019

*/
final def collectM[R1 <: R, E1 >: E, B](e: E1)(pf: PartialFunction[A, ZIO[R1, E1, B]]): ZIO[R1, E1, B] =
self.flatMap {
case v if pf.isDefinedAt(v) => pf(v)

This comment has been minimized.

Copy link
@iravid

iravid Jun 11, 2019

Member

Would it be possible to reformulate this (and rejectM below) using PartialFunction#applyOrElse? That would prevent double-evaluation of pf.

This comment has been minimized.

Copy link
@dcsobral

dcsobral Jun 11, 2019

Ah, yes, applyOrElse! I knew there was something added to avoid double evaluation, but I completely forgot what it was.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@blast-hardcheese

Right, sorry about that, it should be:

self.flatMap {
  case v if ! p(v) => ???
  case v => ZIO.succeed(v)
}
@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

OK, I believe I've addressed all concerns. I had to use andThen to turn ZIO[R, E, E] into ZIO[R, E, A] before applyOrElseing so the result types would line up, other than that this seems to satisfy the constraints.

@iravid
Copy link
Member

left a comment

Could you adjust collectM as well?

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Of course! That one slipped right by me. I've addressed it now.

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Pushed up one more change that addresses pf.andThen(_.flatMap(...)).applyOrElse(..., ...) being all on one line. I thought the formatting tool would have complained or fixed it if I split it onto two lines, but apparently that's permitted.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@blast-hardcheese It seems Circle CI has stalled. Can you open a new pull request so we can get this passing? Thank you!

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

Rebasing this on master would get things rolling CI-wise.

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:adding-cond-and-filterOrElse branch from 47d802c to cb8603c Jun 15, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Updated and squashed commits, 🤞

/**
* Fails with `e` if the predicate fails.
*/
final def filterOrFail[E1 >: E](p: A => Boolean)(e: => E1): ZIO[R, E1, A] =

This comment has been minimized.

Copy link
@jdegoes

jdegoes Jun 15, 2019

Member

The only issue I see is with the parameter ordering for filterOrElse_ and filterOrElse. Ordinarily, the error would be first, because it is most likely to be constant, while the predicate is most likely to vary.

Is there a reason you have the current ordering?

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Jun 16, 2019

Author Contributor

Entirely matching the parameter ordering of the Scala stdlib as well as cats. I'm opening to swapping them if you'd prefer.

This comment has been minimized.

Copy link
@jdegoes

jdegoes Jun 17, 2019

Member

Works for me!

@jdegoes jdegoes merged commit 24a472f into zio:master Jun 17, 2019

7 checks passed

ci/circleci: lint212_jdk8 Your tests passed on CircleCI!
Details
ci/circleci: mdoc Your tests passed on CircleCI!
Details
ci/circleci: test211_js_jdk8 Your tests passed on CircleCI!
Details
ci/circleci: test211_jvm_jdk8 Your tests passed on CircleCI!
Details
ci/circleci: test212_js_jdk8 Your tests passed on CircleCI!
Details
ci/circleci: test212_jvm_jdk8 Your tests passed on CircleCI!
Details
ci/circleci: testdotty_jdk8 Your tests passed on CircleCI!
Details
@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

giphy

Thank you for this contribution, Devon! And congratulations on your first ZIO pull request... hope it won't be the last. 😉

@blast-hardcheese blast-hardcheese deleted the blast-hardcheese:adding-cond-and-filterOrElse branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.