-
-
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) EmptyNameI 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 eallowing me to write
validateName' :: String -> Validation (NonEmpty FormValidationError) UserName
validateName' name = UserName <$> validate (not . null) EmptyName nameOr 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 ethen
validateName :: String -> Validation (NonEmpty FormValidationError) UserName
validateName = validate (not . null) UserName EmptyNameI 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 aWe 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 bBut 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) fswhich 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.