Skip to content

Some comments  #52

@jbpotonnier

Description

@jbpotonnier

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions