Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for multiple validation errors #314 #315

Merged
merged 5 commits into from
Jan 7, 2021

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Dec 30, 2020

Resolves #314

This PR adds support for multiple validation errors.

Here are some interesting points about this implementation.

  • Other than adding to the API support for a list of errors, I left the existing API as is. Internally, I converted each Option and ValueOption to a list.
  • Equality of errors is done by the equality for string list. This means that the order of errors matters. I think that is reasonable.
  • The sample still includes the previous code in which there is only one validation error. I added another text box that includes multiple validation errors.
  • To quickly get this PR working, the Submit message in the sample no longer takes in the parsed data. However, doing so is preferred. As Mark Seemann recently said, validation is a solve problem via applicative functors. Unfortunately, F# doesn't include a Validation<,> monad. I have created one in my project at work. I could add that to the sample but that would also increase the learning curve. Alternatively, one could just parse the input under the assumption it will succeed (via int) because the canExec function returned true. This is not a good practice to do everywhere though.
  • The red box around the text box containing the password is too long on the right side. I don't know why and didn't try to fix it either.

@cmeeren
Copy link
Member

cmeeren commented Dec 31, 2020

The CI build fails, could you investigate?

@TysonMN
Copy link
Member Author

TysonMN commented Dec 31, 2020

Ah, thanks for pointing that out. Will do.

@TysonMN TysonMN force-pushed the multiple_validation_errors branch from 60d71cc to 7bf3052 Compare January 2, 2021 04:43
@TysonMN
Copy link
Member Author

TysonMN commented Jan 2, 2021

CI build passes now. I forgot to update the tests to match the new behavior. I force pushed the needed changes.

@cmeeren
Copy link
Member

cmeeren commented Jan 6, 2021

I pushed a few commits. What do you think?

The rectangle width error was because the error message was wider than the input field. I increased the width of the input field.

@TysonMN
Copy link
Member Author

TysonMN commented Jan 6, 2021

Great improvements. Looks good to me.

Ready to merge?

@cmeeren
Copy link
Member

cmeeren commented Jan 7, 2021

Yep, thanks a lot!

@cmeeren cmeeren merged commit bc8bbca into elmish:master Jan 7, 2021
@TysonMN TysonMN deleted the multiple_validation_errors branch January 7, 2021 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple validation errors
2 participants