wix-incubator / accord

Accord: A sane validation library for Scala
http://wix.github.io/accord/
Other
530 stars 55 forks source link

Add support for .map and .flatMap syntax in the context of .each DSL #140

Closed prSquirrel closed 1 year ago

prSquirrel commented 5 years ago

Makes the following syntax instances valid:

      classroom.students.each map { _.age } should be >= 18
      classroom.students.each map { _.age } map { _.toString } is notEmpty

      classroom.students.each flatMap { _.guardians } is valid
      classroom.students.each flatMap { _.guardians } map { _.age } should be >= 30
      classroom.students.each flatMap { _.guardians.headOption } map { _.age } should be >= 0

      classroom.students.each flatMap { _.guardians } has size.between(0, 2)

I'm not entirely sure supporting flatMap or any other non-structure-preserving operation is the right way to go. Suppose you have val coll = Seq(0, 2) and a function val extend = (i: Int) => Seq(i, i + 1). coll flatMap extend would produce Seq(0, 1, 2, 3). Now, if element 1: Int is invalid, coll.each flatMap extend is valid would refer to [at index 1] - after collection transformation. This works, but may overcomplicate validators.

Also, replaced Traversable[_] with Iterable[_] because of 2.13 collections rework.

prSquirrel commented 5 years ago

52

prSquirrel commented 5 years ago

.map / .flatMap for now, .filter would be trivial to add. Would appreciate your feedback @noam-almog @holograph 🙂

holograph commented 5 years ago

Right out the gate, thank you! At a glance this looks very good, but I'll need to dig a little deeper (plus, only @noam-almog has commit/admin privileges now...).

noam-almog commented 5 years ago

@prSquirrel the build is currently failing, can you take a look ?

prSquirrel commented 5 years ago

@noam-almog fixed!

prSquirrel commented 5 years ago

For some reason 2.11 doesn't infer method to function -> function to validator implicit so I made it an explicit function.

I suppose there was some improvement in 2.12+

prSquirrel commented 5 years ago

Any updates on this? 🙂

holograph commented 5 years ago

By and large, looks very good. One concern I have is around indices on flatmapped arrays; once you change the validated array, indices become rather meaningless (e.g. in the Seq(0, 2) test the result shows the problem at index 1 of the resulting sequence), and error messages become somewhat problematic as well.

It might be a better idea to 1. Report indices on the source sequence or not at all; 2. Maybe force overriding the error message on map/flatMap (for example, if I map via toString and check for nonnegative numbers via startsWith, the error makes no sense).

I’m not clear on how flatMap makes sense from a usage standpoint, but other than the above comments I don’t see any reason not to.