rtfeldman / elm-validate

Convenience functions for validating Elm data.
http://package.elm-lang.org/packages/rtfeldman/elm-validate/latest
BSD 3-Clause "New" or "Revised" License
145 stars 28 forks source link

subject in error and composition #15

Closed stoeffel closed 6 years ago

stoeffel commented 6 years ago

We've got cases where we want to include the subject in the error message. With the current implementation there is no way to construct an error based on a subject.

We could add a function that takes a function to construct the error, like: custom : (subject -> Bool) -> (subject -> error) -> Validator error subject. But this only helps for real custom stuff, because someone might have a use case like:

modelValidator : Validator String Model
modelValidator =
    Validate.all
        [ ifInvalidEmail .email "This is not a valid email address." --- <<< how can we get the e-mail in this error message?
        -- ...
        ]

I think there is no solution to that with this implementation. Yes, we could pass the Model to modelValidator as well, but that makes composition harder and kinda defeats the purpose. We could also make ifTrue : (subject -> Bool) -> (subject -> error) -> Validator subject error. But isn't that almost the same as subject -> List error? This would be simpler if Validator wasn't an opaque type.

modelValidator : Validator String Model
modelValidator =
    Validate.all
        [ \email -> ifInvalidEmail (email ++ This is not a valid email address.") email
        -- ...
        ]

I'm worried that by making Validator opaque we will have to add more and more of these primitives that would otherwise just be functions.

Side note: I was considering making Validator in elm-verify opaque as well, but it would lead to duplicating a lot of the api of Result and would lead to adding many other primitives that would make it harder to learn the api.

rtfeldman commented 6 years ago

ifInvalidEmail could accept a function as its last argument, e.g.

modelValidator : Validator String Model
modelValidator =
    Validate.all
        [ ifInvalidEmail .email (\email -> "This is not a valid email address: " ++ email)
        , ifBlank .firstName "Please enter a first name."
        , ifBlank .lastName "Please enter a last name."
        ]

It wouldn't be useful for ifBlank, so that one wouldn't take a function.

I could see this API being useful on:

Here's a PR that implements those two: https://github.com/rtfeldman/elm-validate/pull/16

Thoughts?

stoeffel commented 6 years ago

We would also need this change for ifTrue, but we want to have access to the subject for creating the error in custom validators. I think with that all our use-cases would work, but I still think it makes composition of Validators harder and would make the API surface bigger, because we need all those new primitives. With other words I don't see the benefit of making Validator opaque, since it's not hiding an implementation detail, or prevents the user from messing with the internals. In my opinion this makes it harder to work with this library, because you need to learn the whole api instead of learning how to compose your program.