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

Change error message for Play JSON to be like Play #35

Merged

Conversation

eddsteel
Copy link
Contributor

@eddsteel eddsteel commented May 6, 2016

In Play JSON, a small number of constant strings are used for validation errors. These can then be localized. It also enables non-Play code to use this message, combined with the path specifying the bad field's location, to generate useful messages.

This change modifies the Play JSON Reads for enums to use strings already in use in Play. Although on their own they are less informative than what's there now, they fit in better with Play and when combined with paths can be used to make more informative messages. (This is our use case. We're not using the play framework but expecting Play JSON behaviour. With current enumeratum behavior we have to prefix-match the error string to generate our errors, but match constant strings for other fields).

If you prefer I can make this an optional thing, like case-insensitivity, but I thought I'd propose this simpler change first.

Use short, constant string names already in use in Play to better
integrate with code expecting that behavior (i.e. Play, or other systems
already using Play JSON).
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9a2c661 on eddsteel:eddsteel/play-validation-message into 6d7e6cf on lloydmeta:master.

@lloydmeta
Copy link
Owner

Hey there, I think think this change is beyond reasonable. I'm not too worried about breaking backwards compatibility because:

  1. This is about changing the error case, so success cases will not change.
  2. As you mentioned, it fits how Play-JSON handles errors better.

Thanks !

@lloydmeta lloydmeta merged commit d5776a3 into lloydmeta:master May 7, 2016
@eddsteel eddsteel deleted the eddsteel/play-validation-message branch May 7, 2016 21:38
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.

3 participants