kowainik / kowainik/validation-selective
Some comments
- Dominant language
- Haskell
- Stars
- 77
- Forks
- 10
- PR merge metrics
- No merged PRs in 30d
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 :
```haskell
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 :
```haskell
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
```haskell
validateName' :: String -> Validation (NonEmpty FormValidationError) UserName
validateName' name = UserName <$> validate (not . null) EmptyName name
```
Or maybe even
```haskell
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
```haskell
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
```haskell
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 :
```haskell
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 :
```haskell
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.
Contributor guide
Research direction
Start by reading the current validateName and validateAll entry points and compare their signatures with the alternatives proposed in the issue. Resolve whether validation should preserve the original value, whether validateAll should return produced values, and whether the API should be added or removed; done requires a decided interface and corresponding project changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- haskell
- Domain
- tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100