Skip to content

Add Semigroup Validation Examples #17

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

Merged
merged 8 commits into from
Aug 1, 2019
Merged

Add Semigroup Validation Examples #17

merged 8 commits into from
Aug 1, 2019

Conversation

jkachmar
Copy link
Contributor

This is my first pass at adding a more detailed Semigroup Validation example, similar in spirit to the examples in the Halogen repository.

If this looks like something that would be valuable to have in the repo, I make a similar example for Semiring Validation and include it in this PR as well.

@chexxor
Copy link

chexxor commented Feb 24, 2018

Anything holding this from being merged? I don't see any problems with merging stuff into an "examples" directory - shouldn't break anything.

@chexxor
Copy link

chexxor commented Apr 19, 2018

Bump

@Profpatsch
Copy link

cc @garyb

@hdgarrood
Copy link
Contributor

I'm going to try to look at this properly over the next week or so.

@hdgarrood
Copy link
Contributor

Really great work on this! I'm sorry it took me a while to get around to looking at it. I have a few comments:

I don't want these examples to be separate bower projects; I think they'll inevitably go out of date. Instead, I'd prefer that we just put the example purs files straight into an examples/ directory and modify the CI script to check that they still compile (e.g. pulp build -I examples).

I'd rather not have the type aliases like type ValidationErrors = Array ValidationError. Can we get rid of those please?

Also, I think we should be showcasing the fact that V only requires a semigroup, and that this allows our types to say more (e.g. that if validation fails, the list of validation errors will be non-empty). In the semigroup example, can we use NonEmptyArray rather than Array please?

Finally, using an array of FormValidationError values for the entire form validation value seems not quite right to me, since the result will be able to contain multiple BadUsername or BadPassword values, whereas it only ought to be able to contain one of each. I think there's a good opportunity to showcase the utility of allowing any semigroup here at the same time as addressing this, via something like

data ValidationField
  = Username
  | Password

newtype FormValidationErrors =
  FormValidationErrors (Map ValidationField (NonEmptyArray ValidationError))

instance semigroupFormValidationErrors :: Semigroup FormValidationErrors where
  append = under2 FormValidationErrors (Map.unionWith (<>))

validateForm :: FormData -> V FormValidationErrors FormResult

What do you think?

On the semiring example, there are a few things which seem not quite right to me:

  • The BadPassword validation messages appear twice in the first example. This may just be a consequence of the free semiring implementation not being commutative (see Free is not a Semiring purescript-semirings#11), but either way, it is clearly not what you would want in real code.
  • The type used for validation errors in that example seems overly complex. In particular, I think having two levels of Free in the type is a little excessive.
  • The comments imply that the reason for choosing semiring over semigroup validation is that you can accumulate errors from two different attempts at validating a particular field. However you can achieve that without too much difficulty using semigroup validation:
    alt :: forall err result. Semigroup err =>
      V err result -> V err result -> V err result
    alt x@(V (Right _)) _ = x
    alt _ y@(V (Right _)) = y
    alt (V (Left err1)) (V (Left err2)) = V (Left (err1 <> err2))
    although to be fair, this is perhaps a little dodgy, since usually the semigroup operation represents a kind of conjunction of errors (this was wrong AND this other thing was wrong).

Because of this, I'd prefer to leave the semiring example out for now; we can revisit it later.

@jkachmar
Copy link
Contributor Author

Hey, sorry about the lag on this. I'll try to rebuild this project with a modern version of purs and address your comments.

@hdgarrood
Copy link
Contributor

No rush at all - it took me over a year to get around to looking at this in the first place so I can't complain 😆

@Profpatsch
Copy link

You can do it!

@jkachmar
Copy link
Contributor Author

Finally got around to this (thanks Compose Unconference!). I needed to use over2 rather than under2, but it seems as if your Map.unionWith suggestion worked just fine!

@hdgarrood
Copy link
Contributor

Looks great, thanks! Would you mind adding a build:example script in package.json and having the .travis.yml script run it after the normal build so that CI can check that this doesn't go out of date?

@hdgarrood
Copy link
Contributor

Thanks very much!

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.

4 participants