"and" and "or" combinators should support short-circuit evaluation

[holograph]

Can you be a bit more specific about what you mean (an example would be best)?

[rangoldberg]

def validator = validator[String] { c =>
    c is aNull or c is notEmpty
  }

This will throw an exception on current implementation when c is null, although it should return Success

[rangoldberg]

The same is relevant for and combinator

[holograph]

Strange. This should work (NotEmpty is null-safe). I'll try to repro and see what's up.

[holograph]

@rangoldberg Actually, scratch that -- this shouldn't even compile, Scala is strict with infix notation so you'll get a compile error like

[error]  found   : String
[error]  required: com.wix.accord.Validator[?]
[error]   val issue122 = validator[String] { c => c is aNull or c is notEmpty }
[error]                                                         ^

I've verified that the correct expression ((c is aNull) or (c is notEmpty)) works as expected. What am I missing?

[rangoldberg]

it worked because notEmpty is null safe, as you mentioned. But this illustrates my point:

object ValidatorExample extends App {
  val value: String = null
  val result = (value is aNull) or (value contains "s")
}

[rangoldberg]

And this:

object ValidatorExample extends App {
  val value: String = null
  val result = (value is notNull) and (value contains "s")
}

[holograph]

@rangoldberg I think I see the problem; the Accord DSL was never intended to be used outside of a validator[T] { t=> ... } block. In other words, is isn't intended to be used directly, and should only be used to define validation rules.

I'll have to see how to make it fail to compile outside the macro, or at least be safe, and will keep the issue open until then.

[holograph]

I re-checked, and this actually fails for a different reason than I expected: you used value contains "s", where contains is not an Accord combinator, but actually a string method -- hence the NPE.

[rangoldberg]

In the following line you apply all predicates, before doing the exist in the following (following) line. Hence, there's no short-circuit. I guess you're doing that for the failures computation.

https://github.com/wix/accord/blob/b62f71541d6b21f595d8ba84840199c978bf5dc9/core/src/main/scala/com/wix/accord/combinators/GeneralPurposeCombinators.scala#L42

[holograph]

Yup, that's correct. I misremembered when we discussed this earlier.