elm-form-decoder

elm-form-decoder copied to clipboard

Hey!

First off, thanks for the great package. Solves so many problems around form validation & the resulting data type.

Say you have a decoder setup like:

import Form.Decoder as D

type Err = MustBeGreaterThan5 | MustBeEven

decoder = 
    D.identity
        |> D.assert (D.minBound MustBeGreaterThan5 5)
        |> D.assert (even MustBeEven)

If we ran this with a few inputs, we'd get:

D.errors decoder 10 -- []
D.errors decoder 7   -- [ MustBeEven ]
D.errors decoder 4   -- [ MustBeGreaterThan5 ]
D.errors decoder 3   -- [ MustBeGreaterThan5 ]

The problem is with the last case of 3. Here, we would expect to see both MustBeGreaterThan5 and MustBeEven in the errors list, since both of them are failures. However, since this is based on applicative style parsing, Form.Decoder must short circuit after the first failure.

To remedy this in my use case, I wrote the following function:

joinResult : Result (List err) () -> Result (List err) () -> Result (List err) ()
joinResult result1 result2 =
    case ( result1, result2 ) of
        ( Err err, Ok () ) ->
            Err err

        ( Ok (), Err err ) ->
            Err err

        ( Err err1, Err err2 ) ->
            Err (err1 ++ err2)

        ( Ok (), Ok () ) ->
            Ok ()


assertMany : List (Decoder.Validator input x) -> Decoder input x value -> Decoder input x value
assertMany validators inputDecoder =
    Decoder.custom <|
        \a ->
            a
                |> Decoder.run inputDecoder
                |> Result.andThen
                    (\val ->
                        let
                            joinedValidators =
                                validators
                                    |> List.map (\decoder -> Decoder.run decoder a)
                                    |> List.foldl joinResult (Ok ())
                        in
                        Result.map (\() -> val) joinedValidators
                    )

Which could be used like:

decoder = 
    D.identity
        |> D.assertMany [ D.minBound MustBeGreaterThan5 5, even MustBeEven ]

D.errors decoder 10 -- []
D.errors decoder 7   -- [ MustBeEven ]
D.errors decoder 4   -- [ MustBeGreaterThan5 ]
D.errors decoder 3   -- [ MustBeGreaterThan5, MustBeEven ]

Essentially, this runs many Validators on an input and joins the errors together. Since we're only validating & not transforming data, this works nicely. However, we could not do something like this for pass or mapN, since later steps require the result type of the pass or mapN.

If this is something that'd make sense to be in the package, I'd be happy to create a PR.

Thanks for the great suggestion. I also like the design of the assertMany function, which takes a list of validators as arguments. Cool. Please create a PR.

One suggestion: How about changing the name asseertMany to assertAll? This might make it easier to convey the intention of "It works like List.all rather than List.any".

Hmm, I just had another thought. I think changing the assert definition to something like the following could work:

--  slight change from above
joinErrors : Result (List err) () -> Result (List err) a -> Result (List err) a
joinErrors result1 result2 =
    case ( result1, result2 ) of
        ( Err err, Ok _ ) ->
            Err err

        ( Ok (), Err err ) ->
            Err err

        ( Err err1, Err err2 ) ->
            Err (err1 ++ err2)

        ( Ok (), Ok val ) ->
            Ok val

assert : Validator input err -> Decoder input err a -> Decoder input err a
assert v (Decoder f) =
    custom <|
        \a ->
            joinErrors (run v a) (f a)

Instead of short circuiting on errors for assert, we can run the Validator anyways. Since assert isn't really an applicative (since it does not change the type of the Decoder's a), we can run both with the same input. That is, we don't need to run the Validator a err with the result of the Decoder input err a since the latter doesn't change the type of a like pass does.

Then, with the original example I believe it would work:

decoder = 
    D.identity
        |> D.assert (D.minBound MustBeGreaterThan5 5)
        |> D.assert (even MustBeEven)

D.errors decoder 10 -- []
D.errors decoder 7   -- [ MustBeEven ]
D.errors decoder 4   -- [ MustBeGreaterThan5 ]
D.errors decoder 3   -- [ MustBeGreaterThan5, MustBeEven ]

I haven't tested this, but I can later today. If this works, then I'm inclined to do this method instead since it seems more intuitive & doesn't add another function. What do you think?

If you prefer the other method, the name assertAll works for me 😃

Thanks for the interesting suggestion. I feel that the previous policy adding assertMany is superior.

Reason 1: It is not appropriate to change the behavior of functions in a package that has already been released. This is not a fatal reason, but changing the behavior of a function is harmful and confusing. Developers who have been using form-decoder have to rewrite all their existing codes when updating form-decoder version. It would even cause nasty bugs.

Reason 2: The method of adding assertMany is better for fine-tune the behavior of assertions. The current behavior of the assert function is not necessarily bad. It allows for a kind of "early return" or "pruning". This property is even valuable in real-world application development. Changing the assert function to the one you suggest would make this kind of processing impossible. Rather, I believe that it would be more beneficial to allow developers to fine-tune the behaviour of assertions with the assertMany function.

This is current my answer, but please let me know if there is anything I have missed or misunderstood. 🌷

Totally see where you're coming from. assertAll solve my use case and is straightforward to use. I'll submit a PR when I can!