Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
77 changes: 28 additions & 49 deletions core/common/src/main/scala/net/liftweb/common/Box.scala
Original file line number Diff line number Diff line change
Expand Up @@ -518,30 +518,6 @@ 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 @@ -575,18 +551,18 @@ sealed abstract class Box[+A] extends Product with Serializable{
*/
def asA[B](implicit m: Manifest[B]): Box[B] = Empty

/**
* 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] = alternative

/**
* Returns an `[[scala.collection.Iterator Iterator]]` over the value
* contained in this `Box`, if any.
*/
def elements: Iterator[A] = Iterator.empty

/**
* 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 }

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.

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.

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.

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

@Bhashit Bhashit Sep 11, 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.

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.

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.

I think that's fine.


/**
* Get a `java.util.Iterator` from the Box.
*/
Expand Down Expand Up @@ -814,30 +790,39 @@ sealed abstract class Box[+A] extends Product with Serializable{
}

/**
* Returns a `Full` box containing the results of applying `flipFn` to this box if it is a `Failure`,
* `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.

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.

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?

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.

Yeah, that sounds pretty helpful. Will do so.

*
* @example {{{
*
* // Returns Full("alternative") because the partial function covers the case.
* Failure("error") flip { case Failure("error", Empty, Empty) => "alternative" }
* Full("error") transform { case Full("error") => Full("alternative") }
*
* // Returns Empty because the partial function doesn't cover the case.
* Failure("another-error") flip { case Failure("error", Empty, Empty) => "alternative" }
* // Returns Full(1), this Full box unchanged, because the partial function doesn't cover the case.
* Full(1) transform { case Full(2) => Failure("error") }
*
* // Returns this Failure("another-error") unchanged because the partial function doesn't cover the case.
* Failure("another-error") transform { case Failure("error", Empty, Empty) => Full("alternative") }
*
* // Returns Full("alternative") for an Empty box since `partialFn` is defined for Empty
* Empty flip { case Empty => "alternative" }
* Empty transform { case Empty => Full("alternative") }
*
* // Returns Empty because the partial function is not defined for Empty
* Empty flip { case Failure("error", Empty, Empty) => "alternative" }
*
* // Returns Empty for a Full box
* Full(1) flip { case _ => Failure("error") }
* Empty transform { case Failure("error", Empty, Empty) => Full("alternative") }
*
* }}}
*/
def flip[B](flipFn: PartialFunction[EmptyBox, B]): Box[B] = this match {
case e: EmptyBox if flipFn.isDefinedAt(e) => Full(flipFn(e))
def transform[B >: A](transformFn: PartialFunction[Box[A], Box[B]]): Box[B] = {
transformFn.applyOrElse(this, (thisBox: Box[A]) => thisBox)
}

/**
* 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
}

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.

}
Expand All @@ -852,8 +837,6 @@ final case class Full[+A](value: A) extends Box[A] {

override def openOr[B >: A](default: => B): B = value

override def or[B >: A](alternative: => Box[B]): Box[B] = this

override def exists(func: A => Boolean): Boolean = func(value)

override def forall(func: A => Boolean): Boolean = func(value)
Expand Down Expand Up @@ -920,8 +903,6 @@ sealed abstract class EmptyBox extends Box[Nothing] with Serializable {

override def openOr[B >: Nothing](default: => B): B = default

override def or[B >: Nothing](alternative: => Box[B]): Box[B] = alternative

override def filter(p: Nothing => Boolean): Box[Nothing] = this

override def ?~(msg: => String): Failure = Failure(msg, Empty, Empty)
Expand Down Expand Up @@ -956,8 +937,6 @@ 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
72 changes: 45 additions & 27 deletions core/common/src/test/scala/net/liftweb/common/BoxSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,6 @@ 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)
}
"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")
Expand All @@ -164,8 +160,21 @@ class BoxSpec extends Specification with ScalaCheck with BoxGenerator {
Full("Hermione") collect { case "Albus" => "Dumbledore"} must beEmpty
}
}
"define a 'transform' method that takes a PartialFunction to transform this box into another box" in {
"If the partial-function is defined for this box, returns the result of applying the partial function to it" in {
Full(404) transform {
case Full(x: Int) if x != 200 => Failure("Server error")
} must_== Failure("Server error")
}
"If the partial-function is not defined for this box, returns itself unchanged" in {
Full("Intended Result") transform {
case _: EmptyBox => Full("Alternative")
case Full("Unexpected Result") => Full("Alternative")
} must_== Full("Intended Result")
}
}
"define a 'flip' method returning Empty" in {
Full(1) flip { case _ => "alternative" } must_== Empty
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
Expand Down Expand Up @@ -313,25 +322,26 @@ 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 'flip' method that takes a PartialFunction to transform this Empty box into something else" in {
"If the partial-function is defined for Empty, returns a full box containing the result of applying the partial function to it" in {
Empty flip { case Empty => "Return Of The Jedi" } must_== Full("Return Of The Jedi")
Empty flip {
case Failure("The Phantom Menace", Empty, Empty) => "Attack"
case Empty => "Return Of The Jedi"
} must_== Full("Return Of The Jedi")
"define a 'transform' method that takes a PartialFunction to transform this Empty box into another box" in {
"If the partial-function is defined for Empty, returns the result of applying the partial function to it" in {
Empty transform {
case Failure("error", Empty, Empty) => Full("failure-alternative")
case Empty => Full("alternative")
} must_== Full("alternative")
}
"If the partial-function is not defined for Empty, returns Empty" in {
Empty flip { case Failure("The Phantom Menace", Empty, Empty) => "Return Of The Jedi" } must_== Empty
Empty transform { case Failure("The Phantom Menace", Empty, Empty) => Full("Return Of The Jedi") } 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
}
Expand Down Expand Up @@ -392,22 +402,30 @@ 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)
}
"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 'flip' 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") flip { case Failure("The Phantom Menace", Empty, Empty) => "Return Of The Jedi" } must_== Full("Return Of The Jedi")
Failure("The Phantom Menace") flip { case Failure("The Phantom Menace", Empty, Empty) => Failure("Attack") } must_== Full(Failure("Attack"))
"define a 'transform' method that takes a PartialFunction to transform this Failure into another box" in {
"If the partial-function is defined for this Failure, returns the result of applying the partial function to it" in {
Failure("The Phantom Menace") transform {
case Failure("The Phantom Menace", Empty, Empty) => Full("Return Of The Jedi")
} must_== Full("Return Of The Jedi")

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