Add transform and flip methods. #1864

Bhashit · 2017-06-17T15:13:40Z

Add transform and flip methods.

transform allows callers to take a Box and, via PartialFunction, turn it into any
other Box. If the PartialFunction fails, the original Box is returned.

flip allows callers to take a Box and, if it is an EmptyBox, flip it into a Full box
with a specific type. If it is a Full, an Empty is returned, flipping the box from
full to empty or vice versa.

Bhashit · 2017-06-17T15:17:08Z

Will be adding tests for this as well. Just wanted to get this in early for a review.

Bhashit · 2017-06-18T06:59:56Z

The suggestion by codacy is about the fact that mapFailure includes the word Failure, which is also the name of one of the classes that has that method.

farmdawgnation · 2017-06-18T13:52:58Z

🙄 Codacy.

Feel free to ignore it's output if it's about something silly. I feel like there are many, many times where I need to disregard one of its suggestions because it's not actionable.

Shadowfiend · 2017-06-24T20:04:04Z

Looks like you removed the only collect method, not a duplicate? mapFailure looks sweet.

Bhashit · 2017-06-25T03:25:10Z

Thanks @Shadowfiend. I'll add the tests for mapFailure today.

About the collect method: I think I removed the duplicate collect method I had added. We already have an implementation here.

Shadowfiend · 2017-06-25T18:46:50Z

About the collect method

< rubs eyes > lol, guess I'd missed that! Need to make a note to myself to change that to use applyOrElse.

Shadowfiend · 2017-06-25T20:57:07Z

As I'm working on lift-formality, it occurred to me it could also be particularly useful to add a collectFailure, since pattern matching is used heavily when dealing with ParamFailure. Thoughts?

Shadowfiend

Couple of quick things.

Shadowfiend · 2017-06-25T21:12:46Z

core/common/src/test/scala/net/liftweb/common/BoxSpec.scala

@@ -152,14 +152,17 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
        Full(Empty).flatten must_== Empty
      }
    }
+    "define a 'mapFailure' method returning itself." in {
+      Full(1).mapFailure(identity) must_==  Full(1)


Even if mapFailure didn't map just over a failure, this would return Full(1). How about a parameter that returns some standard Failure?

Yeah, I did that get that niggling feeling when I wrote this. You are right. That could be confusing. I'll change it to something more appropriate.

Shadowfiend · 2017-06-25T21:12:57Z

core/common/src/test/scala/net/liftweb/common/BoxSpec.scala

@@ -298,14 +301,17 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
    "define a 'flatten' method returning Empty" in {
      Empty.flatten must beEmpty
    }
+    "define a 'mapFailure' method returning itself." in {
+      Empty.mapFailure(identity) must beEmpty


Ditto re: above.

Shadowfiend · 2017-06-25T21:13:25Z

core/common/src/test/scala/net/liftweb/common/BoxSpec.scala

-    "return a itself when asked for its status with the operator ?~" in {
+    "define a 'mapFailure' method that transforms it into another Failure instance." in {
+      val exception = new Exception("transformed")
+      Failure("error", Empty, Empty) mapFailure { _ => Failure("new-error", Full(exception), Empty) } must_=== Failure("new-error", Full(exception), Empty)


Any particular need for === here?

Bhashit · 2017-06-26T03:47:10Z

Need to make a note to myself to change that to use applyOrElse

I'll do that.

it could also be particularly useful to add a collectFailure, since pattern matching is used heavily when dealing with ParamFailure. Thoughts?

I'll push an implementation for review.

…h Empty or Full. Replace must_=== with must_==

…`collect`.

Bhashit · 2017-06-26T04:52:20Z

core/common/src/main/scala/net/liftweb/common/Box.scala

-    if (pf.isDefinedAt(value)) Full(pf(value))
-    else Empty)
-  }
+  final def collect[B](pf: PartialFunction[A, B]): Box[B] = filter(pf.isDefinedAt).map(pf)


@Shadowfiend thought this was more readable than

final def collect[B](pf: PartialFunction[A, B]): Box[B] = flatMap { value => pf.andThen(Full.apply).applyOrElse(value, _ => Empty) }

I do think this version will result in a double-application of the PartialFunction guards/pattern matches (a problem that the previous version in the repo also shared):

@ def slap(in: String) = println(s"Uh-oh ${in}"); false defined function slap @ val booyan: PartialFunction[String, Int] = { case "boom" if slap("jam") => 5; case _ => 8 } booyan: PartialFunction[String, Int] = <function1> @ Full("boom").filter(booyan.isDefinedAt).map(booyan) Uh-oh jam Uh-oh jam res13: Box[Int] = Full(8) @ booyan.andThen(Full.apply).applyOrElse("boom", { _: String => Empty }) Uh-oh jam res12: Box[Int] = Full(8)

So while I agree this is slightly more readable, I think you nailed it with the original alternative formulation.

Bhashit · 2017-07-09T09:27:38Z

@Shadowfiend just pushed an initial version of collectFailure. If it looks good, I'll add some comments and tests, and if it doesn't, I'll make the necessary changes.

…ements

Shadowfiend · 2017-07-09T23:44:30Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+  final def collectFailure[B](pf: PartialFunction[Failure, Failure]): Box[Failure] = this match {
+    case f: Failure if pf.isDefinedAt(f) => Full(pf(f))
+    case _ => Empty
+  }


It feels like this should be a PartialFunction[Failure, B] and result in a Box[B], no?

That was my first instinct as well. Almost sent a comment asking about these two signatures. I think I'll go with that one then. Now that you said it, it makes more sense. Convert specific cases of Failure into something else.

Add tests and comments

Bhashit · 2017-07-10T03:43:47Z

@Shadowfiend ~~I think this might be ready for review. I.e. now it contains all the methods that I set out to add, along with their respective tests.~~

Never mind, I forgot about the flip method that you mentioned. Will add that one tonight.

Shadowfiend · 2017-07-22T19:19:52Z

@Bhashit flip can be a separate PR if you'd like. Up to you.

Wrap a couple of functions in braces. Replace if-else for checking partial function applicability with an apply-or-else. Add a `flip` method to Box

Bhashit · 2017-07-23T05:03:13Z

@Shadowfiend added the flip method. And corrected the collect implementation. It turns out that Box has a couple of apply implementations that take a PartialFunction and do what we need. So, used those. And used applyOrElse in those existing methods. The remaining codacy complaint is about the method mapFailure having "Failure" in its name.

Shadowfiend · 2017-07-23T19:12:20Z

core/common/src/main/scala/net/liftweb/common/Box.scala

@@ -731,7 +757,7 @@ sealed abstract class Box[+A] extends Product with Serializable{


  /**
-   * If the `Box` is `Full`, apply the transform function `f` on the value `v`;
+   * If the `Box` is `Fuvall`, apply the transform function `f` on the value `v`;


Shadowfiend · 2017-07-23T19:14:06Z

core/common/src/test/scala/net/liftweb/common/BoxSpec.scala

+    "define a 'mapFailure' method returning itself." in {
+      val failure = Failure("new-error", Empty, Empty)
+      Full(1).mapFailure(_ => failure) must_==  Full(1)
+    }


This looks like it's a duplicate of one below?

Oh wait jk I just remembered how this spec is structured heh.

Shadowfiend · 2017-07-23T19:21:14Z

core/common/src/test/scala/net/liftweb/common/BoxSpec.scala

+    "define a 'collectFailure' method that takes a PartialFunction to transform this Failure into something else" in {
+      "If the partial-function is defined for this Failure, returns a full box containing the result of applying the partial function to it" in {
+        Failure("The Phantom Menace") collectFailure { case Failure("The Phantom Menace", Empty, Empty) => "Return Of The Jedi" } must_== Full("Return Of The Jedi")
+        Failure("The Phantom Menace") collectFailure { case Failure("The Phantom Menace", Empty, Empty) => Failure("Attack") } must_== Full(Failure("Attack"))


So a random interesting observation here tying into your question about making flip take a PartialFunction… Now that we've added flip, collectFailure is really just a special subcase of flip… Do we want to keep both? Or do we, as you initially suggested, make flip take a PartialFunction, and drop collectFailure?

Also of interest is that mapFailure and collectFailure work very differently despite their very similar names (probably the reason why initially you defined collectFailure as PartialFunction[Failure,Failure] instead of PartialFunction[Failure,B]).

Not sure what the best way forward is, but I'm slightly concerned about these two things… Would be interested in thoughts/ideas before we commit to this setup as it currently stands. Going with mapFailure + flip seems like a good way to handle the common use cases without adding the oddity of collectFailure.

I do like flip that takes a PartialFunction, in which case, collectFailure is indeed a special case of flip. I'll go ahead and make the changes this Saturday.

Also, on a related note, let me know if we later need to adapt these methods (or any existing ones) to the new hierarchy you are implementing. I'd be happy to take that up.

Sounds good. I expect this PR will go in before mine, and if that's the case I'll take on the task of pulling it into my refactoring. If mine goes in first, I'll let you take it on :)

Remove `collectFailure` as `flip` covers the case it was written to handle

Bhashit · 2017-08-06T04:35:07Z

@Shadowfiend changes pushed. Although, the following case of flip did make me somewhat uncomfortable.

Empty flip { case Failure("error", Empty, Empty) => "alternative" }

This would return Empty. The thing that bothered me was that flip sounds definitive, whereas collect has a well-known meaning that it will only apply in some cases. Although, I think that should be okay.

Bhashit · 2017-09-03T04:44:30Z

Just a gentle reminder (not sure if I'm supposed to do that): I think this is ready for review. The only remaining codacy complaint is about the Failure having a method mapFailure ("You should not name methods after their defining object").

Shadowfiend · 2017-09-05T20:08:56Z

You are well within your rights to poke me, yes :) I've been short on time, but will have another look soon. Can you remove the WIP from the PR title?

Shadowfiend · 2017-09-06T16:19:21Z

Although, the following case of flip did make me somewhat uncomfortable.

This is a very, very fair concern... Enough so that I think it maybe calls into question the idea of giving flip a PartialFunction.

That point made me think a little bit about mapFailure as well. The trouble with that name is that it implies a tie to map, but map deals with the contents of a Box, not with the Box itself. I'm wondering if we tilt this a little bit… We keep flip as a total function that can flip EmptyBox into Full. And we introduce def transform[A](transformFn: PartialFunction[Box[A], Box[A]): Box[A]. transform can transform the box in any way it wants, as long as the contained value type remains unchanged (to change the contained value type you'd have to use collect or map).

This definition of transform encompasses what we currently consider mapFailure (by simply passing a PartialFunction[Failure, Failure]). It also encompasses the PartialFunction variant of flip. And it leaves flip to be as definitive as it sounds.

Sorry to come back with yet another “let's rework this yet again”---let me know what you think, and if you have time to look at doing it. If it sounds good to you, I can also make the change and let you review it. Whatever works! I think it would be great to see this land by M2 next week.

EDIT to say: transform basically also acts as a generalized form of or---or can be defined as transform { case _: EmptyBox => other }. It essentially allows some introspective manipulation for the box along a transformation chain.

Bhashit · 2017-09-07T03:11:45Z

Sounds good. I'll try to get that in over this weekend. Please don't worry about revising. That's what gets us a good API eventually.

Shadowfiend · 2017-09-07T12:15:21Z

Absolutely, but I try to keep in mind how long these feedback cycles have been… I appreciate your patience!

Bhashit · 2017-09-10T05:01:42Z

I'm somewhat embarrassed that I didn't spot this until I started working on it: since Box is covariant in type A, and the return type param of PartialFunction is contravariant, we can't use PartialFunction[Box[A], Box[A]] here.

We'll need to use something like:

transform[B >: A](transformFn: PartialFunction[Box[A], Box[B]]): Box[B]

One caveat with that signature is that, unless A and B are the same type, the returned box will be some common super-type of both (and in may cases, that turns out to be Any or AnyRef).

What do you think? Any other ideas, or is this the way we want to go?

I'm pushing some changes, as I think this might be okay since users will probably know they are doing. (just for review, we can revise as needed)

Remove the `mapFailure` method as it's convered by the `transform` method. Implement `or` using `transform`. Change `flip` to take a function, instead of a `PartialFunction`

Shadowfiend

since Box is covariant in type A, and the return type param of PartialFunction is contravariant

ಠ_ಠ

That said, defining it as [B >: A] is really strictly more flexible than the alternative, so I think that's okay.

Shadowfiend · 2017-09-11T13:57:29Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+    * Return this Box if `Full`, or the specified alternative if it is
+    * empty. Equivalent to `Option`'s `[[scala.Option.orElse orElse]]`.
+    */
+  def or[B >: A](alternative: => Box[B]): Box[B] = transform { case _: EmptyBox => alternative }


While this is quite clean, I wonder if we should leave or as-is. The JVM will be able to turbo-inline the simple definitions of or, whereas this definition will probably (though not necessarily) be near-impossible to optimize. Given how common it is to call or, I'm tempted to say we should leave it as-is.

Hmm. Yeah, I didn't think it along those lines. I'll change that back.

Do you think keeping the or abstract in the base Box class will make much of a difference? I mean, that way, there's one implementation in Full and one in EmptyBox. It sort of seems cleaner.

I think that's fine.

Shadowfiend · 2017-09-11T14:00:04Z

core/common/src/main/scala/net/liftweb/common/Box.scala

-    * `ParamFailure` or `Empty`, and `flipFn` is defined for it. Returns `Empty` if this box is `Full`.
-    * In other words, it "flips" the full/empty status of this Box.
+    * Transforms this box using the `transformFn`. If `transformFn` is defined for this box,
+    * returns the result of applying `transformFn` to it. Otherwise, returns this box unchanged.


I think we should at a note here that if you are looking to change the value inside a Full box, you should really be using map, and if you're looking to change a Failure or Empty into a Full, you should really be using flip.

There's enough complexity in Box that providing some helpful guides in some of this method-specific documentation starts making sense for me. What do you think?

Yeah, that sounds pretty helpful. Will do so.

Add an additional explanatory comment on `transform` about its intended use

Shadowfiend · 2017-09-11T15:16:26Z

core/common/src/main/scala/net/liftweb/common/Box.scala

@@ -793,6 +793,10 @@ sealed abstract class Box[+A] extends Product with Serializable{
    * Transforms this box using the `transformFn`. If `transformFn` is defined for this box,
    * returns the result of applying `transformFn` to it. Otherwise, returns this box unchanged.
    *
+    * If you want to change the content of a `Full` box, using [[map]] or [[collect]] might be better
+    * suited to that purpose. If you want to convert an `Empty`, `Failure` or a `ParamFailure` into a
+    * `Full` box, you should use [[flip]].


I'd put backticks around flip, map, and collect (in addition to the [[ and ]]).

Generally, as a part of the review, I would take the words I'd put backticks to mean that I (as the author) should make that change. But since you approved, I am guessing you are making that change? Let me know if I should do it, and I'll push the change.

Shadowfiend

Ok, I am 100% sold here. Tremendous work, and thanks for sticking with it. Hard to believe how few lines of code were added… Turned out to be mostly analysis work!

🙇

Bhashit · 2017-09-11T15:36:41Z

I think you did the most of the hard work here though. So, a 'thank you' is in order. 🙇‍♂️

Shadowfiend · 2017-09-11T15:38:58Z

Yeah, the request was for you to make the change, but I approved the PR because there's really no reason to block on it :)

Bhashit · 2017-09-11T15:58:54Z

If/when this gets merged, do you need to adapt the new methods in your branch? More specifically, you need an extra hand with that? Otherwise, I'm planning to work on the vanilla JS artifacts this week.

Shadowfiend · 2017-09-11T15:59:34Z

I'll disentangle my branch, no worries. Have to finish up test coverage in there before starting on that anyway.

Shadowfiend · 2017-09-17T15:33:50Z

If anyone else wants to have a glance here and sign off, I'm good on merging… I'll probably merge later this week otherwise.

Shadowfiend · 2017-09-25T02:59:57Z

Bit late, buuuut in we go!

Shadowfiend · 2017-09-25T03:04:19Z

Tweaked the description and title to show what we ended up with ;)

Add collect and mapFailure methods. WIP

e2150e7

Remove duplicated collect method

f9ecab8

Tests for the mapFailure method

ba011d3

Shadowfiend requested changes Jun 25, 2017

View reviewed changes

Bhashit added 2 commits June 26, 2017 09:25

change mapFailure tests to use a valid Failure param when testing wit…

5c607f9

…h Empty or Full. Replace must_=== with must_==

Change the collect function to be a bit more readable. Add tests for …

42bbf08

…`collect`.

Bhashit commented Jun 26, 2017

View reviewed changes

8000

Bhashit added 2 commits June 26, 2017 10:23

Corrected spacing

a5b3972

Initial pass at collectFailure method

2cc1ac8

Merge branch 'master' into issue-1856-box-combinator-and-other-improv…

b797621

…ements

Shadowfiend reviewed Jul 9, 2017

View reviewed changes

Shadowfiend mentioned this pull request Jul 9, 2017

Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Draft

2 tasks

Correction in collectFailure method signature.

a10b78e

Add tests and comments

Redefine the collect method to use an existing apply function.

d90ccff

Wrap a couple of functions in braces. Replace if-else for checking partial function applicability with an apply-or-else. Add a `flip` method to Box

Shadowfiend reviewed Jul 23, 2017

View reviewed changes

Implement flip to take a partial-function.

52e6f17

Remove `collectFailure` as `flip` covers the case it was written to handle

Bhashit changed the title ~~Add collect and mapFailure methods. WIP~~ Add collect and mapFailure methods. Sep 6, 2017

Implement transform method to transform a Box into another Box.

94cc62c

Remove the `mapFailure` method as it's convered by the `transform` method. Implement `or` using `transform`. Change `flip` to take a function, instead of a `PartialFunction`

Shadowfiend reviewed Sep 11, 2017

View reviewed changes

Rollback 'or' to use its own simpler implementation.

f504a82

Add an additional explanatory comment on `transform` about its intended use

Shadowfiend reviewed Sep 11, 2017

View reviewed changes

Shadowfiend approved these changes Sep 11, 2017

View reviewed changes

Put backticks around the method-name links

783bf6e

Shadowfiend added this to the 3.2.0-M3 milestone Sep 17, 2017

Shadowfiend merged commit 50a556c into lift:master Sep 25, 2017

Shadowfiend changed the title ~~Add collect and mapFailure methods.~~ Add transform and flip methods. Sep 25, 2017

Add transform and flip methods. #1864

Add transform and flip methods. #1864

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!