kowainik / kowainik/validation-selective

Some comments

Open
#52 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.