kowainik/validation-selective

Some comments

jbpotonnier opened this issue · 2 comments

Hello to both of you,
thanks for your work that helps a lot to understand Haskell concepts.

Not really a bug here, but some comments based on your tutorial, that are hopefully useful.

First, isn't it error prone to do this :

validateName name = UserName name <$ failureIf (null name) EmptyName

I see a risk of validating a value and building another value. I see <$ as a smell, and would rather use <$>.

Here, my intuition would be to have a helper validate like this :

validate :: (a -> Bool) -> e -> a -> Validation (NonEmpty e) a
validate p  e x = if p x then Success x else failure e

allowing me to write

validateName' :: String -> Validation (NonEmpty FormValidationError) UserName
validateName' name = UserName <$> validate (not . null) EmptyName name

Or maybe even

validate :: (a -> Bool) -> (a -> b) -> e -> a -> Validation (NonEmpty e) b
validate p c e x = if p x then Success (c x) else failure e

then

validateName :: String -> Validation (NonEmpty FormValidationError) UserName
validateName = validate (not . null) UserName EmptyName

I think it reads quite well, and also avoids building a wrong result.
One could also imagine some variants of validate, taking id or const () instead of the constructor

What do you think ?


Another thing I would like to comment is the validateAll function.

It is defined as

validateAll
    :: forall e b a f
    .  (Foldable f, Semigroup e)
    => f (a -> Validation e b)
    -> a
    -> Validation e a

We are throwing the b values produced by the validations, and I think the result type is surprising too.

I would rather expect, hoping that the validators all returns the same value :

validateAll
    :: forall e b a f
    .  (Foldable f, Semigroup e)
    => f (a -> Validation e b)
    -> a
    -> Validation e b

But then, what would b be when the foldable is empty ?

So if we really want to provide a validateAll function, I would rather do :

validateAll :: Semigroup e => NonEmpty (a -> Validation e b) -> a -> Validation e b
validateAll fs a = head <$> traverse ($ a) fs

which is only keeping the first produced value, so it might not be so nice.
Maybe validateAll is not needed, and *> would be clearer.

Here again, I would be happy to know your point of you.

After all, this one doesn't look too bad, I think :

validateAll ::
  (Foldable t, Semigroup e) =>
  t (a -> Validation e b) ->
  (a -> c) ->
  a ->
  Validation e c
validateAll fs c a = c a <$ traverse_ ($ a) fs

validatePassword :: String -> Validation (NonEmpty FormValidationError) Password
validatePassword = validateAll [validateShortPassword, validatePasswordDigit] Password

I think you're touching on something really interesting with validate; the type (a -> Bool) -> e -> a -> Validation (NonEmpty e) a captures our intuition that the failure condition usually has something to do with -- or is somehow based on -- the input data. While this has been the case every time I've used the package, it's at least conceivable that sometimes it won't be the case. In other words, it seems that validate is less general than failureIf.

For the record, I think this is a good thing! In complex cases, it helps protect you from testing the wrong data. So, I'd be all for adding this combinator to the library. (Also, I like this type better than the validate that takes an (a -> b); feels redundant with the functor instance.)