Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
79 changes: 70 additions & 9 deletions core/common/src/main/scala/net/liftweb/common/Box.scala
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ sealed trait BoxTrait {
* @return A `Full` containing the transformed value if
* `pf.isDefinedAt(value)` and `Empty` otherwise.
*/
def apply[InType, OutType](pf: PartialFunction[InType, OutType])(value: InType): Box[OutType] =
if (pf.isDefinedAt(value)) Full(pf(value)) else Empty
def apply[InType, OutType](pf: PartialFunction[InType, OutType])(value: InType): Box[OutType] = {
apply(value)(pf)
}

/**
* Apply the specified `PartialFunction` to the specified `value` and return
Expand All @@ -180,8 +181,9 @@ sealed trait BoxTrait {
* @return A `Full` containing the transformed value if
* `pf.isDefinedAt(value)` and `Empty` otherwise.
*/
def apply[InType, OutType](value: InType)(pf: PartialFunction[InType, OutType]): Box[OutType] =
if (pf.isDefinedAt(value)) Full(pf(value)) else Empty
def apply[InType, OutType](value: InType)(pf: PartialFunction[InType, OutType]): Box[OutType] = {
pf.andThen(Full.apply).applyOrElse(value, (_: InType) => Empty)
}

/**
* This implicit transformation allows one to use a `Box` as an `Iterable` of
Expand Down Expand Up @@ -516,6 +518,30 @@ sealed abstract class Box[+A] extends Product with Serializable{
*/
def foreach[U](f: A => U): Unit = {}

/**
* If this box is a `Failure`, returns a `Failure` box that results from passing this box
* to `fn`. Otherwise, returns this box. Used for transforming Failures without having to
* cover all three cases of Box, which is required by `map`.
*
* `map` is only applied if the box is full, `mapFailure` is only applied if this box is a `Failure`.
*
* @example {{{
* // Returns `Failure("Changed failure")` because this box is a failure.
* Failure("Original Failure") mapFailure { failed => Failure("Changed failure") }
*
* // Returns Full("some-value") on which it was called
* Full("some-value") mapFailure { failure => Failure("Changed failure") }
*
* // Returns this Empty instance
* Empty mapFailure { case value => value }
* }}}
*
* @param fn the function that will be used to transform the Failure if this box is a failure.
* @return A `Failure` instance that results from applying `fn` if this box is a `Failure`, otherwise
* the same instance on which it was called.
*/
def mapFailure(fn: Failure => Failure): Box[A] = this

/**
* If this box is `Full` and contains an object of type `B`, returns a `Full`
* of type `Box[B]`. Otherwise, returns `Empty`.
Expand Down Expand Up @@ -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`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whoops!

* otherwise, just return the value untransformed.
*
* The transform function is expected to be a function that will take the
Expand Down Expand Up @@ -773,10 +799,8 @@ sealed abstract class Box[+A] extends Product with Serializable{
* If the partial function is defined at the current Box's value, apply the
* partial function.
*/
final def collect[B](pf: PartialFunction[A, B]): Box[B] = {
flatMap(value =>
if (pf.isDefinedAt(value)) Full(pf(value))
else Empty)
final def collect[B](pf: PartialFunction[A, B]): Box[B] = flatMap { value =>
Box(value)(pf)
}

/**
Expand All @@ -788,6 +812,41 @@ sealed abstract class Box[+A] extends Product with Serializable{
final def collectFirst[B](pf: PartialFunction[A, B]): Box[B] = {
collect(pf)
}

/**
* If this box is a `Failure` and if `pf` is defined for this `Failure` instance, returns
* a `Full` box containing the result of applying `pf` to this `Failure`, otherwise, returns
* `Empty`.
*
* @example {{{
* // Returns Full("alternative") because the partial function covers the case.
* Failure("error") collectFailure { case Failure("error", Empty, Empty) => "alternative" }
*
* // Returns Empty because the partial function doesn't cover the case.
* Failure("another-error") collectFailure { case Failure("error", Empty, Empty) => "alternative" }
*
* // Returns Empty for an Empty box
* Empty collectFailure { case _ => Failure("error") }
*
* // Returns Empty for a Full box
* Full(1) collectFailure { case _ => Failure("error") }
*
* }}}
*/
final def collectFailure[B](pf: PartialFunction[Failure, B]): Box[B] = this match {
case f: Failure if pf.isDefinedAt(f) => Full(pf(f))
case _ => Empty
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@Bhashit Bhashit Jul 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


/**
* Returns a `Full` box containing the results of applying `flipFn` to this box if it is a `Failure`,
* `ParamFailure` or `Empty`. Returns `Empty` if this box is `Full`. In other words, it "flips" the
* full/empty status of this Box.
*/
def flip[B](flipFn: EmptyBox => B): Box[B] = this match {
case e: EmptyBox => Full(flipFn(e))
case _ => Empty
}
}

/**
Expand Down Expand Up @@ -904,6 +963,8 @@ sealed case class Failure(msg: String, exception: Box[Throwable], chain: Box[Fai

override def map[B](f: A => B): Box[B] = this

override def mapFailure(f: (Failure) => Failure): Box[A] = f(this)

override def flatMap[B](f: A => Box[B]): Box[B] = this

override def isA[B](cls: Class[B]): Box[B] = this
Expand Down
66 changes: 61 additions & 5 deletions core/common/src/test/scala/net/liftweb/common/BoxSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
Full(1) reduceLeft {(x: Int, y: Int) => x + y} must_== 1
}
"be used as an Option" in {
Full(1) orElse Some(2) must_== Some(1)
Empty orElse Some(2) must_== Some(2)
Full(1) orElse Some(2) must beSome(1)
Empty orElse Some(2) must beSome(2)
}
"be implicitly defined from an Option. The openOrThrowException method can be used on an Option for example" in {
Some(1).openOrThrowException("This is a test") must_== 1
Expand Down Expand Up @@ -152,14 +152,32 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
Full(Empty).flatten must_== Empty
}
}
"define a 'mapFailure' method returning itself." in {
val failure = Failure("new-error", Empty, Empty)
Full(1).mapFailure(_ => failure) must_== Full(1)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

"define a 'collect' method that takes a PartialFunction to transform its contents" in {
"If the partial-function is defined for the contents of this box, returns a full box containing the result of applying that partial function to this Box's contents" in {
Full("Albus") collect { case "Albus" => "Dumbledore"} must_== Full("Dumbledore")
}
"If the partial-function is not defined for the contents of this box, returns Empty" in {
Full("Hermione") collect { case "Albus" => "Dumbledore"} must beEmpty
}
}
"define a 'collectFailure' method returning Empty" in {
Full(1) collectFailure { case _ => Failure("error") } must_== Empty
}
"define a 'flip' method returning Empty" in {
Full(1) flip { _ => "No data found" } mustEqual Empty
}
"define an 'elements' method returning an iterator containing its value" in {
Full(1).elements.next must_== 1
}
"define a 'toList' method returning a List containing its value" in {
Full(1).toList must_== List(1)
}
"define a 'toOption' method returning a Some object containing its value" in {
Full(1).toOption must_== Some(1)
Full(1).toOption must beSome(1)
}
"return itself if asked for its status with the operator ?~" in {
Full(1) ?~ "error" must_== Full(1)
Expand Down Expand Up @@ -298,14 +316,30 @@ 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 {
val failure = Failure("new-error", Empty, Empty)
Empty.mapFailure(_ => failure) must beEmpty
}
"define a 'collect' method returning Empty" in {
Empty collect { case _ => "Some Value" } must beEmpty
}
"define a 'collectFailure' method returning Empty" in {
Empty collectFailure { case _ => Failure("error") } must_== Empty
}
"define a 'flip' method returning a Full box" in {
Empty flip {
case Empty => "flipped-empty"
case _ => "flipped-failure"
} mustEqual Full("flipped-empty")
}
"define an 'elements' method returning an empty iterator" in {
Empty.elements.hasNext must beFalse
}
"define a 'toList' method returning Nil" in {
Empty.toList must_== Nil
}
"define a 'toOption' method returning None" in {
Empty.toOption must_== None
Empty.toOption must beNone
}
"return a failure with a message if asked for its status with the operator ?~" in {
Empty ?~ "nothing" must_== Failure("nothing", Empty, Empty)
Expand Down Expand Up @@ -358,7 +392,29 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
Failure("error", Empty, Empty) flatMap {x: String => Full(x.toString)} must_== Failure("error", Empty, Empty)
Failure("error", Empty, Empty).flatten must_== Failure("error", Empty, Empty)
}
"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)
}
"define a 'collect' method returning itself" in {
Failure("error", Empty, Empty) collect { case _ => "Some Value" } must_== Failure("error", Empty, Empty)
}
"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"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@Bhashit Bhashit Jul 27, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 :)

}
"If the partial-function is not defined for this Failure, returns Empty" in {
Failure("Attack Of The Clones") collectFailure { case Failure("The Phantom Menace", Empty, Empty) => "Return Of The Jedi" } must_== Empty
}
}
"define a 'flip' method returning a Full box" in {
Failure("error", Empty, Empty) flip {
case Empty => "flipped-empty"
case _: Failure => "flipped-failure"
} mustEqual Full("flipped-failure")
}
"return itself when asked for its status with the operator ?~" in {
Failure("error", Empty, Empty) ?~ "nothing" must_== Failure("error", Empty, Empty)
}
"create a new failure with a chained message if asked for its status with the operator ?~!" in {
Expand Down