Skip to content

Some comments  #52

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

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions