scalactic: flatWithGood?

[Imran Rashid]

Hi,

I've just started using scalactic for Or and Every, and mostly I'm very happy.  But I've run into a couple of cases where the usage feels funky, probably I'm just not quite getting it yet.

The first one is when I want to do more error handling *inside* a call to withGood.  eg., something like:

val age = parseAge()
val height = parseHeight()

withGood(age,height){ case(a,h) =>
  val expHeartRate: Or[Int,One[ErrorMessage]] = parseExpHeartRate()
  expHeartRate.map{hr =>
    ...
  }
}

I don't want to call parseExpHeartRate() until after I know I've got a good age & height.  withGood helps me do that nicely.  But, now I don't have a good way to deal w/ the error handling inside -- b/c parseExpHeartRate() might not work, the result type is now

Or[ Or[T,One[ErrorMessage]], Every[ErrorMessage]]

but overall I want

Or[T, Every[ErrorMessage]]

I feel like I want the equivalent of flatMap, eg. flatWithGood, or maybe just a flatten method on Or.  Is there a better way to do this?

thanks,
Imran

[Bill Venners]

Hi Imran,

Sorry I didn't notice your post was pending until now.

Would parseExpHeartRate return something that you need to construct your T type? I.e., you can't construct the T until you have the result of parseExpHeartRate in hand?

Or can you construct your T type with just "name" and "age", but then you want to further validate that T instance with parseExpHeartRate?

Bill

--
You received this message because you are subscribed to the Google
Groups "scalatest-users" group.
To post to this group, send email to scalate...@googlegroups.com
To unsubscribe from this group, send email to
scalatest-use...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/scalatest-users?hl=en
ScalaTest itself, and documentation, is available here:
http://www.artima.com/scalatest
---
You received this message because you are subscribed to the Google Groups "scalatest-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scalatest-use...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Bill Venners
Artima, Inc.
http://www.artima.com

[Bill Venners]

Hi Imran,

In case you can't construct a T unless you have the heart rate, one idea I had was to use withGood make an intermediate function that given the heartRate could produce a Person. Here's a working example:

case class Person(name: String, age: Int, heartRate: Int)

def parseName(input: String): String Or One[ErrorMessage] = {
  val trimmed = input.trim
  if (!trimmed.isEmpty) Good(trimmed) else Bad(One(s""""${input}" is not a valid name"""))
}

def parseAge(input: String): Int Or One[ErrorMessage] = {
  try {
    val age = input.trim.toInt
    if (age >= 0) Good(age) else Bad(One(s""""${age}" is not a valid age"""))
  }
  catch {
    case _: NumberFormatException => Bad(One(s""""${input}" is not a valid integer"""))
  }
}

def parseHeartRate(input: String): Int Or One[ErrorMessage] = {
  println("Executed expensive parseHeartRate method")
  try {
    val heartRate = input.trim.toInt
    if (heartRate >= 0) Good(heartRate) else Bad(One(s""""${heartRate}" is not a valid heart rate"""))
  }
  catch {
    case _: NumberFormatException => Bad(One(s""""${input}" is not a valid integer"""))
  }
}

import Accumulation._

def parsePerson(inputName: String, inputAge: String, inputHeartRate: String): Person Or Every[ErrorMessage] = {
  val name = parseName(inputName)
  val age = parseAge(inputAge)
  val heartRateToPerson = withGood(name, age) { (nm, ag) => (hr: Int) => Person(nm, ag, hr) }
  for {
    personWith <- heartRateToPerson
    heartRate <- parseHeartRate(inputHeartRate)
  } yield personWith(heartRate)
}

Here's some examples in the REPL:

scala> parsePerson("Fred", "22", "99")
Executed expensive parseHeartRate method
res0: org.scalactic.Or[Person,org.scalactic.Every[org.scalactic.ErrorMessage]] = Good(Person(Fred,22,99))

scala> parsePerson("Fred", "22", "oops")
Executed expensive parseHeartRate method
res1: org.scalactic.Or[Person,org.scalactic.Every[org.scalactic.ErrorMessage]] = Bad(One("oops" is not a valid integer))

scala> parsePerson("Fred", "-22", "oops")
res2: org.scalactic.Or[Person,org.scalactic.Every[org.scalactic.ErrorMessage]] = Bad(One("-22" is not a valid age))

scala> parsePerson("", "-22", "oops")
res3: org.scalactic.Or[Person,org.scalactic.Every[org.scalactic.ErrorMessage]] = Bad(Many("" is not a valid name, "-22" is not a valid age))

The type of heartRateToPerson in parsePerson is:

Int => Person Or Every[ErrorMessage]]

In other words it is an Or whose Good type is a function that can construct a person given a heart rate. Its Bad type is accumulated error messages. A for expression short circuits, so that if the first generator results in a Bad, it doesn't execute the next generator.

personWith <- heartRateToPerson

So if heartRateToPerson is a Bad already, it doesn't execute the next generator:

heartRate <- parseHeartRate(inputHeartRate)

The withGood invocation is a bit tricky looking:

withGood(name, age) { (nm, ag) => (hr: Int) => Person(nm, ag, hr) }

Because we're passing name and age in, "withGood(name, age)...", we need to give withGood a function that takes two parameters of types String and Int. That's the "(nm, ag) => " part. The rest is the *result* of the function passed to withGood, which is " (hr: Int) => Person(nm, ag, hr)". Well that's a function that takes an Int (the heart rate) and produces a Person using nm, ag, and hr. So withGood will either return a Good function of that type, or Every error message produced by parsing the name and the age.

Not sure how well this scales up. I'll think about it some more, but flatMap is indeed under the covers the method that is being called by the for expression. So I think your intuition was on the right track looking for a flatMap.

Bill

[Imran Rashid]

Hi Bill,

thanks for the response.  Sorry for giving a half-baked example and making you guess my intentions.  In my actual use case, the delayed error checking depends on having valid data from the earlier steps.  Its as if "parseHeartRate" really required a name & age to work, eg:

def parseHeartRate(input: String, name: String, age: Int): Int = ...

(non-sensical in this example, I know).  But the use case you came up with -- that parseHeartRate was expensive and should be avoided -- is just as good.  The basic idea is that I want to mix & match accumulating a bunch of errors together sometimes, and also short-circuit at other times.

I understand what you are doing with your example, but it still seems a little klunky to me.  It did make me think of another solution, which I like a lot, that uses zip in one for expression.  Keeping the other definitions the same, this makes parsePerson look like:

def parsePerson(inputName: String, inputAge: String, inputHeartRate: String): Person Or Every[ErrorMessage] = {
  val name = parseName(inputName)
  val age = parseAge(inputAge)

  for {
    nameAge <- name.zip(age)
    (n,a) = nameAge
    heartRate <- parseHeartRate(inputHeartRate)
  } yield Person(n,a,heartRate)
}


It probably seems a little weird that I don't combine the first two lines into

(n,a) <- name.zip(age)

but, that gives this compile error:

[error]  type mismatch;
[error]  found   : Boolean(true)
[error]  required: org.scalactic.Validation[?]
[error]         (n, a) <- name.zip(age)
[error]                                     ^

which was confusing at first, but when I remembered that pattern matching in a for expression got desugared to a filter, I saw why:

val g = Good("hi")
val g2 = Good("there")
scala> g.zip(g2).filter{case(a,b) => true}
<console>:16: error: type mismatch;
 found   : Boolean(true)
 required: org.scalactic.Validation[?]
              g.zip(g2).filter{case(a,b) => true}

The filter() method on Or doesn't have a signature that is compatible with pattern matching.  I dunno if there is anything that can be done to make that work.  The extra line isn't really a big deal, it would just be nice to prevent the initial confusion.

thanks
Imran

[Bill Venners]

Hi Imran,

Sorry for the latency in responding. Was focused on a frustrating implicit problem yesterday.

On Tue, Jul 22, 2014 at 6:33 AM, Imran Rashid <im...@quantifind.com> wrote:

Hi Bill,

thanks for the response.  Sorry for giving a half-baked example and making you guess my intentions.  In my actual use case, the delayed error checking depends on having valid data from the earlier steps.  Its as if "parseHeartRate" really required a name & age to work, eg:

def parseHeartRate(input: String, name: String, age: Int): Int = ...

(non-sensical in this example, I know).  But the use case you came up with -- that parseHeartRate was expensive and should be avoided -- is just as good.  The basic idea is that I want to mix & match accumulating a bunch of errors together sometimes, and also short-circuit at other times.

I understand what you are doing with your example, but it still seems a little klunky to me.  It did make me think of another solution, which I like a lot, that uses zip in one for expression.  Keeping the other definitions the same, this makes parsePerson look like:

def parsePerson(inputName: String, inputAge: String, inputHeartRate: String): Person Or Every[ErrorMessage] = {
  val name = parseName(inputName)
  val age = parseAge(inputAge)

  for {
    nameAge <- name.zip(age)
    (n,a) = nameAge
    heartRate <- parseHeartRate(inputHeartRate)
  } yield Person(n,a,heartRate)
}

Nice.
 

It probably seems a little weird that I don't combine the first two lines into

(n,a) <- name.zip(age)

but, that gives this compile error:

[error]  type mismatch;
[error]  found   : Boolean(true)
[error]  required: org.scalactic.Validation[?]
[error]         (n, a) <- name.zip(age)
[error]                                     ^

which was confusing at first, but when I remembered that pattern matching in a for expression got desugared to a filter, I saw why:

val g = Good("hi")
val g2 = Good("there")
scala> g.zip(g2).filter{case(a,b) => true}
<console>:16: error: type mismatch;
 found   : Boolean(true)
 required: org.scalactic.Validation[?]
              g.zip(g2).filter{case(a,b) => true}

The filter() method on Or doesn't have a signature that is compatible with pattern matching.  I dunno if there is anything that can be done to make that work.  The extra line isn't really a big deal, it would just be nice to prevent the initial confusion.

Actually that's on purpose, and there is something that can be done to make it work that involves an implicit conversion. I wasn't sure whether to document that technique for the 2.0 release, or whether to provide something that would have users make a typeclass instance instead. I decided to wait until a user actually hit it to start talking about it, and to my recollection you are the first! Anyway, let me give some background.

 

When a validation fails, you should get an instance of the "bad" type. But when a filter fails, all you have is Boolean false. So how would one decide on the "bad" type?

Try had this design question, and given their bad type (Failure) is Throwable, they just picked NoSuchElementException. That seems reasonable, but when someone gets that it might be quite non-obvious how the NoSuchElementException got there. Here's an example:

scala> scala.util.Success("y'all").filter(_.length > 5)
res13: scala.util.Try[String] = Failure(java.util.NoSuchElementException: Predicate does not hold for y'all)

Scalaz's Validation also had this same design question, and they said, hey I know, we'll just stick the Monoid zero in there for the "bad" type (also called Failure), therefore requiring up front that a Monoid instance exist for the bad type. I think that's really wrong, because for example, if your bad type is a Stirng error message, you'll just get an extremely frustrating-for-the-user non-obvious empty string if a filter fails. Here's an example:

scala> scalaz.Success[String,String]("y'all").filter((s: String) => s.length > 5)
res5: scalaz.Validation[String,String] = Failure()

The Scalactic Or case is similar to the scalaz Validation case, in that there's not one known "bad" type as in Try. Because the "bad" type is Throwable for Try, they could pick an instance, and they picked NoSuchElementException. In Or, as in scalaz Validation, you can specify any Bad type.

So instead of picking something like a Monoid instance, which is too general in my opinion, I make users say exactly what they want by making the filter and withFilter methods take function from the Good type to a Validation of the Bad type. I.e., it is:

def filter[C >: B](f: G => Validation[C]): G Or C

Not:

def filter(f: G => Boolean): G Or ?

This works fine in for expressions when you make explicit filters:

scala> def isRound(i: Int): Validation[ErrorMessage] =
     |   if (i % 10 == 0) Pass else Fail(i + " was not a round number")
isRound: (i: Int)org.scalactic.Validation[org.scalactic.ErrorMessage]

scala> for (i <- Good(3) if isRound(i)) yield i
res1: org.scalactic.Or[Int,org.scalactic.ErrorMessage] = Bad(3 was not a round number)

scala> for (i <- Good(30) if isRound(i)) yield i
res2: org.scalactic.Or[Int,org.scalactic.ErrorMessage] = Good(30)

The trouble is that sometimes, as you discovered, for expression desugaring will produce filter invocations with the G => Boolean signature. So this doesn't compile (on purpose, since there's no good default "bad" instance for any type B in my opinion):

scala> import Accumulation._
import Accumulation._

scala> val pair = Good(3).zip(Bad(One("oops")))
pair: org.scalactic.Or[(Int, Nothing),org.scalactic.Every[String]] = Bad(One(oops))

scala> for ((a, b) <- pair) yield a
<console>:21: error: type mismatch;

 found   : Boolean(true)
 required: org.scalactic.Validation[?]

              for ((a, b) <- pair) yield a
                             ^

The way to fix it is to declare what you think the default "bad" type is for a type by defining an implicit conversion from Boolean to Validation[B]:

scala> import scala.language.implicitConversions
import scala.language.implicitConversions

scala> implicit def cvt(b: Boolean): Validation[String] = if (b) Pass else Fail("hmm")
cvt: (b: Boolean)org.scalactic.Validation[String]

scala> for ((a, b) <- pair) yield a
res2: org.scalactic.Or[Int,Object] = Bad(One(oops))

Voila. So that's the technique. The design question I had for Scalactic in general is still, should I just document this technique, or should I provide a general implicit conversion in the Validation companion object that takes a typeclass instance of some type I make up to mean: this is the desired default instance for a particular Bad type. That would mean that users could make typeclass instances instead of an implicit conversion, which I think would be better. Now that I've had some time to think about it, I think this would be best probably, but I'd like to hear opinions. It would look like:

Fresh REPL:

scala> import scala.language.implicitConversions
import scala.language.implicitConversions

scala> import Accumulation._
import Accumulation._

scala> case class DefaultBad[B](defaultBad: B)
defined class DefaultBad

// This implicit would go into the Validation companion

scala> implicit def generalCvt[B](b: Boolean)(implicit ev: DefaultBad[B]): Validation[B] = if (b) Pass else Fail(ev.defaultBad)
generalCvt: [B](b: Boolean)(implicit ev: DefaultBad[B])org.scalactic.Validation[B]

scala> val pair = Good(3).zip(Bad(One("oops")))
pair: org.scalactic.Or[(Int, Nothing),org.scalactic.Every[String]] = Bad(One(oops))

scala> for ((a, b) <- pair) yield a
<console>:22: error: type mismatch;

 found   : Boolean(true)
 required: org.scalactic.Validation[?]

              for ((a, b) <- pair) yield a
                             ^

// Then to solve the compiler error you declare the default Bad value for

// a type like this:

scala> implicit val badStr = DefaultBad("houston we have a problem")
badStr: DefaultBad[String] = DefaultBad(houston we have a problem)

// Again your code will work when for desugaring generates withFilter calls:

scala> for ((a, b) <- pair) yield a
res3: org.scalactic.Or[Int,Object] = Bad(One(oops))

// But you could also use boolean filter expressions now explicitly now

// when the Bad type is String:

scala> for (i <- Good(30) if i % 10 == 0) yield i
res4: org.scalactic.Or[Int,String] = Good(30)

scala> for (i <- Good(3) if i % 10 == 0) yield i
res5: org.scalactic.Or[Int,String] = Bad(houston we have a problem)

So in short, it is a similar usage to scalaz's approach, but instead of Monoid, which I think is too general to be useful for this, it is a specific typeclass, DefaultBad. The other way I depart from the scalaz folks is I'm not stuck on the notion that there should be only one typeclass instance per type. There isn't one default Bad[String] instance that make sense in all cases. You need to be able to define instances in the scopes they are needed and where they make sense, and DefaultBad would let you do that.

So that's my thinking currently. I'd like to hear feedback. In the meantime you can use the implicit conversion technique I showed you, or even just do the second more general approach and make a DefaultBad of your own for the time being.

Thanks.

Bill

[Imran Rashid]

Hi Bill,

thanks for the really detailed response!

On Wed, Jul 23, 2014 at 11:58 AM, Bill Venners <bi...@artima.com> wrote:

On Tue, Jul 22, 2014 at 6:33 AM, Imran Rashid <im...@quantifind.com> wrote:

The filter() method on Or doesn't have a signature that is compatible with pattern matching.  I dunno if there is anything that can be done to make that work.  The extra line isn't really a big deal, it would just be nice to prevent the initial confusion.

Actually that's on purpose, and there is something that can be done to make it work that involves an implicit conversion. I wasn't sure whether to document that technique for the 2.0 release, or whether to provide something that would have users make a typeclass instance instead.

 

...
 

The design question I had for Scalactic in general is still, should I just document this technique, or should I provide a general implicit conversion in the Validation companion object that takes a typeclass instance of some type I make up to mean: this is the desired default instance for a particular Bad type.

 

So that's my thinking currently. I'd like to hear feedback.

I dunno if I am the right person to ask ... I'm often very reluctant to use implicits, which might put me in the minority of scala users & your audience.  (I think I've just moved beyond the initial fascination with them.)  But, since you asked for my opinion, here goes :)

Honestly, I'm not a fan of *either* approach you propose.  I don't like the idea of that type of conversion coming from an implicit, it just seems like it will cause more confusion than it helps.

Let me explain where I'm coming from.  I truly don't care that as things are now, I have one more line of code.  I think LOC is a terrible metric for code complexity -- often fancy one-liners dramatically increase code complexity.  I'm not just worried about code complexity as I write the code initially, I'm also worried about clarity later on during maintenance, whether its by me or somebody else.

So, the initial error that I complained about:

[error]  type mismatch;
[error]  found   : Boolean(true)
[error]  required: org.scalactic.Validation[?]
[error]         (n, a) <- name.zip(age)
[error]                                ^

certainly leads to some confusion, but the tools I need to work through it are just an understanding of how for-expressions works.  That's a core part of scala syntax that a dev needs to be able to work through

Suppose instead we had this lurking somewhere in our web of implicits:

implicit val badStr = DefaultBad("houston we have a problem")

that would avoid the initial confusion during compilation, and also as long as use followed the Good path ... but then one day the unsuspecting dev suddenly sees "houston we have a problem" and thinks, where in the world did that come from?  The amount of surprise and the work to track it down is a much bigger time sync / context switch.  That's a lot worse than a more common, but easily solved compilation error.  In general, I want to limit implicits to use-cases where they can easily be tracked down (or at least, with a little help from Intellij).

We discussed internally using scalaz's validation briefly, but actually decided against it, because it would introduce a huge amount of complexity.  That complexity is especially bad when debugging / tweaking existing code, partially because of the introduced web of implicits.  I really like that scalactic is useful *and* easy to understand.  I know this is one tiny implicit but I've come to adopt a policy of resistance.

I guess the one change that I *would* ask for is just documenting this problem, and perhaps even the 2-line solution, on the usage page.

anyway, that's my 2 cents.  I won't be offended if you ignore my feedback, nor would I stop using and liking scalactic whichever way you go :)

[Bill Venners]

Hi Imran,

Thanks for your feedback. I think your concerns are valid, and they
touch on why I didn't do anything about it in the 2.0 release. My
concern wasn't so much implicits in general as you've expressed. It
was more that the implicit DefaultBad would be fixing something that
is also "implicit" in a way: the rewrite of for expressions. I figure
most folks have a fairly vague mental model of how for expressions are
desugared. Since they can't see this desugared code explicitly, it
would really be hard for them to understand how the implicit is being
applied if they ever needed to do so.

At the end of the day, implicits can help reduce boilerplate code, and
if you have a lot of cases where you want write for expressions that
generate filter calls, it can be annoying to have to write the extra
code. So I'll keep thinking about it, and I'd be curious what others
think. You can today fix that with an implicit, so one thing I could
do instead of adding something like DefaultBad to the library is just
document how it would be done. I should at least put something in the
documentation about this kind of situation, no matter what.

Bill

[Imran Rashid]

On Thu, Jul 24, 2014 at 5:12 PM, Bill Venners <bi...@artima.com> wrote:

 I figure
most folks have a fairly vague mental model of how for expressions are
desugared. Since they can't see this desugared code explicitly, it
would really be hard for them to understand how the implicit is being
applied if they ever needed to do so.

yes, I agree with that assessment.  I've certainly struggled with for-expressions many times, I suppose I made it sound much too easy in my last message.  I can definitely see your dilemma and why you want to save a user from a painful for-expression as well.  But I guess that's why I'm even more scared of an implicit *within* a for-expression.  And I'm especially scared of implicit conversions which happen without some explicit "trigger", a la ".asScala" in JavaConverters, as opposed to the automatic one from JavaConversions.

Good luck choosing the design :) I'm also curious what other users will think ...

[Greg Soltis]

For a little bit of outside perspective, we've used an explicit implicit method to solve a related problem.

We are using scalaz's \/ type, aliased to Result[Error, Value]. So you might have a method like parseInt(i: Int): Result[ErrorType, Int], and to return a correctly parsed int, you would return Value(i), vs Error("some error message").

These work nicely in a for-comprehension, as long as the error types match. One problem we ran into, which I think is similar to what you are running into here, was when we were dealing with a method that didn't return a Result, but instead returned an Option or Try. We did not want to have a default error conversion, since the point of the exercise was to be able to provide helpful error messages. We ended up writing an implicit class for both Option and Try and defining an orError method on them. The orError would be called immediately on the result of the function that returned an Option, for instance, and the developer would supply an appropriate error message for the None case. For example:

def lookupResource(s: String): Option[Resource] = ???

val result: Result[String, Resource] = for {

  resource <- lookupResource(name) orError { s"Resource $name not found" }

} yield ....

The orError method looks like:

implicit class OptionExtras[R](opt: Option[R]) {

  def orError[E](f: => E): Result[E, R] = {

    opt match {

      case Some(value) => Value(value)

      case None => Error(f)

    }

  }

}

with a similar construct for Try.

So perhaps it would be possible to implement something with a PartialFunction to do the filter, combined with an orError-like method to handle the case where the filter fails? It seems like the key would be doing it in such a way that the de-sugaring doesn't produce boolean output.

For the zip example, something like:

for {

  (name, age) <- goodAs[(String, Int)] { name.zip(age) } orError { "bad name or age" }

}

Just some food for thought, and I apologize if it's nonsensical. I'm just coming up to speed on scalactic and this is an adaptation of what we do in our code base.