Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

prSquirrel
Copy link

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
Copy link
Author

#52

@prSquirrel
Copy link
Author

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

@holograph
Copy link
Contributor

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
Copy link
Collaborator

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

@prSquirrel
Copy link
Author

@noam-almog fixed!

Copy link
Contributor

@holograph holograph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the "Scala 2.11 fix" changes seem necessary, what am I missing here?

@prSquirrel
Copy link
Author

prSquirrel commented Feb 18, 2019

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
Copy link
Author

Any updates on this? 🙂

@holograph
Copy link
Contributor

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.

@prSquirrel prSquirrel closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants