davegurnell / checklist

Validation library for Scala.
Apache License 2.0
47 stars 11 forks source link

Rule Builders #7

Closed Jacoby6000 closed 7 years ago

Jacoby6000 commented 7 years ago

Allows creation of "rule builders" which I described a bit in #6.

Essentially, you can now have structures like this:

case class InputFoo(untrimmed: String, maybeEmptyList: List[Int])
case class ValidatedFoo(trimmed: String, nonEmptyList: NonEmptyList[Int])

val rule = 
    Rule.builder[InputFoo]
      .check("untrimmed", _.untrimmed)(Rule.trimString)
      .check("maybeEmptyList", _.maybeEmptyList)(Rule.nonEmptyList)
      .build[ValidatedFoo]

rule(InputFoo("bar ", List("baz"))) // Ior.Right(ValidatedFoo("bar", NonEmptyList("baz")))
rule(InputFoo("bar", List()) // Ior.Left(NonEmptyList(ErrorMessage("Must not be empty", PField("maybeEmptyList", PNil))))

The important thing here, is that transformations caused by the rules are preserved.

Motivation

Provided some user input X, a validated version of X occupies a subset of all values for X. If your validator is a function that ultimately returns X, you've lost the context about X which tells you it is validated. If your validation function instead converts to a more refined type, like Y, then Y can accurately portray values for X that have been validated. From there on, your code can just accept instances of Y, and you can rest assured that you're only dealing with validated data. Additionally, a raw X may not be parameterized on it's type at all, while Y can be an HKT of sorts.

codecov[bot] commented 7 years ago

Codecov Report

Merging #7 into develop will increase coverage by 2.65%. The diff coverage is 82.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop       #7      +/-   ##
===========================================
+ Coverage    76.81%   79.47%   +2.65%     
===========================================
  Files            9       10       +1     
  Lines          138      151      +13     
===========================================
+ Hits           106      120      +14     
+ Misses          32       31       -1
Impacted Files Coverage Δ
checklist/src/main/scala/checklist/Path.scala 73.68% <ø> (ø) :arrow_up:
checklist/src/main/scala/checklist/ToMessage.scala 66.66% <ø> (ø) :arrow_up:
...klist/src/main/scala/checklist/CheckedSyntax.scala 100% <ø> (ø) :arrow_up:
...hecklist/src/main/scala/checklist/RuleMacros.scala 0% <ø> (ø) :arrow_up:
...n/scala/checklist/refinement/RuleHListSyntax.scala 100% <100%> (ø)
checklist/src/main/scala/checklist/Message.scala 100% <100%> (ø) :arrow_up:
checklist/src/main/scala/checklist/Rule.scala 81.25% <72.72%> (+3.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8a468a9...61d3e66. Read the comment docs.

Jacoby6000 commented 7 years ago

@davegurnell It looks like it's been awhile since you've looked at this repo. I know the PR has only been up for a day, but I'd really like to see this (or something similar) get added. It's really the only thing stopping me from using it.

dispalt commented 7 years ago

for the record, I really like this and use checklist.

davegurnell commented 7 years ago

@Jacoby6000 sorry I missed this and your issue -- this is awesome! I'd never thought about the type refinement angle -- it's a great idea.

I've put a couple of small questions inline. Can you take a look at them? I'm more or less happy to just merge this, though.

Jacoby6000 commented 7 years ago

Something I'm not entirely happy about with this is using .check as the method, but there are other places where .field is used... Both names are pretty arbitrary and have much different behavior, so I'm not really sure what to do about that. It might get confusing to end users.

Also, in other places you use some macros to figure out the path, but here I'm not doing that.. I'm explicitly taking it in. I'd like to keep it explicit, since it makes the behavior less surprising and a bit more flexible (your function for contramap may pull multiple fields from the parent object). But I'm not sure how you feel about that, or if you have better ideas.

Jacoby6000 commented 7 years ago

Oh, also I messed with your build file a bit, which is why I changed the view bounds to implicit params, and removed some old imports. I hope you don't mind that. Just wanted to make that clear, since it touched some unrelated things.

davegurnell commented 7 years ago

Good point about the check method name. I didn't notice the inconsistency. I think it makes sense to keep the method names (i.e. field) consistent. The builder / build names you've used make sense.

I'm pretty happy with my macro approach because it's fast to compile. It doesn't give exhaustivity like the HList approach does, but then exhaustivity isn't always what one wants. However, I'm also a fan of choosing the right tool for the job. Shapeless makes sense for building full-class rules like in your examples. I'm happy to let the user decide what they want to do.

How about this. We organise the code so your builder/build pattern comes from a separate package with a separate import, a bit like circe.generic.auto. Then we can document that package separately. Users will be less confused because they're specifically opting in to using a derivation-like approach. What do you think?

I'm happy with your changes to the build file. Cross-building against Scala 2.12 is great. I may fiddle with the scalacOptions a bit if they cause me problems, but I doubt they will.

Jacoby6000 commented 7 years ago

Sounds good to me. Since it's going to be opt-in, I'll make it a separate sub-project, so people can pull in checklist without getting the shapeless stuff.

davegurnell commented 7 years ago

Ok sounds good. Thanks very much for working on this, by the way. It's really great!

Jacoby6000 commented 7 years ago

No problem! Thanks for giving me something to build on. I'm far too lazy to do the initial ground work 😉.

Before merging, you might want to add some methods to the check stuff that use lenses. I'm not sure though. I'm not super familiar with monocle, so I didn't use them. I'm sure they could be helpful though!

davegurnell commented 7 years ago

I'll merge this now. I'm keen to tweak some method names in the future. I'll open issues to discuss and do it in a minor release, though.

davegurnell commented 7 years ago

Merged and release as 0.3.0. Again, there are some minor tweaks to the syntax that I'd like to make in the future, but I don't want to get in the way of you using this stuff.

davegurnell commented 7 years ago

Thanks! :+1:

Jacoby6000 commented 7 years ago

@davegurnell I can see that you published 0.3.0, but it looks like the refinement stuff didn't make it to maven. Can you take a look when you get a chance?

Jacoby6000 commented 7 years ago

Oh nevermind. I was looking for checklist-refinement but it's in there as just refinement

davegurnell commented 7 years ago

Ah. That's something to fix next release then :) On Fri, 1 Sep 2017 at 19:31, Jacob Barber notifications@github.com wrote:

Oh nevermind. I was looking for checklist-refinement but it's in there as just refinement

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/davegurnell/checklist/pull/7#issuecomment-326653135, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOI596T6-ZnsOe9u3YxI19iOODuviXfks5seE1qgaJpZM4PFKyl .