-
-
Notifications
You must be signed in to change notification settings - Fork 10
Description
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.