add filter and withFilter alias on DecodeResult

[chrislewis]

Original motivation was wanting a slightly more humane way of filtering json that parsed successfully, yet may not represent a valid value. For example, if a string value is used to represent case objects and we need to make sure this value maps to one of them, pattern guards are convenient means to do so.

[seanparsons]

Would it be possible to have a slightly more generalised test in CodecSpecification for this? Otherwise it looks good to me, but I'd prefer to have a test that deals with both positive and negative cases.

[markhibberd]

Hey @chrislewis, I am not really sure what you intend the behaviour to be, but as it is defined, there are a few subtle problems - primarily you will lose any error message or context if you hit the guard (because it is going to call monoid zero - so it error will be empty string and empty history). And, I am a bit hesitant to introduce a gotcha like that.

As an alternative, I would be happier with a validate combinator on DecodeResult and/or DecodeJson, something like:

DecodeJson(c => for {
  name <- (c --\ "name").as[String]
  age <- (c --\ "age").as[Int].validate(_ > 30, c, "Age should be more than 30")
} yield Person(name, age))

Also, open to other suggestions, but I would really prefer it if at least the cursor history is maintained - the ability to provide a sensible error is also useful.

[chrislewis]

Hi @markhibberd - I actually like that much better! Still new to the code but I'm guessing preserving the history in filter would be easy, and a default message (value failed predicate or some such) would probably be easy as well (perhaps in terms of your suggested combinator). I'll look into that.

[chrislewis]

@markhibberd in your proposed sketch validate takes the Cursor; is that intentional? I may have misunderstood the role of the DecodeResult's result value, but it seems to me that validate could be implemented as:

  def validate(f: A => Boolean, error: String) =
    flatMap { a =>
      if (f(a)) this
      else DecodeResult(-\/(error -> (history getOrElse CursorHistory(Nil))))
    }

If the predicate fails, the result is the provided error string and the cursor history, which will be empty if there was no history. If that's what you had in mind then filter follows:

  def filter(f: A => Boolean) =
    validate(f, "Predicate filtered value")

Obviously that's a less useful message, but it least it's sane. Have I misunderstood anything?

[markhibberd]

@chrislewis The cursor argument was intentional, because dropping the history means you lose the connection of what failed (you will know why, but not necessarily what field). I passed it explicitly to solve that problem, however it would be possible for the success case of DecodeResult to maintain the cursor state in the success case as well - basically DecodeResult[A](history: CursorHistory, result: String \/ A) - which would allow what you suggest + maintain history. This is obviously a much larger change - but I would be happy with it, because it makes everything a lot nicer. (if it becomes too difficult, I could maybe do that change first to make it easier - but it wouldn't happen for a week or so).

[chrislewis]

Ah, I see. So if it were to be implemented without changing the structure of DecodeResult, validate would be:

  def validate(f: A => Boolean, c: HCursor, error: String) =
    flatMap { a =>
      if (f(a)) this
      else DecodeResult(-\/(error -> c.history))
    }

And filter wouldn't be sane because it would drop any history. With the more comprehensive change where history is always available, we would have:

  def validate(f: A => Boolean, error: String) =
    flatMap { a =>
      if (f(a)) this
      else DecodeResult(history, -\/(error))
    }

And filter would then benefit from this new history preserving behavior. I might have a go at this just for the sake of increasing my familiarity with the library.

[markhibberd]

@chrislewis Yep, that all sounds right.

[chrislewis]

Awesome, that's the better path then.