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
Passing context information in errors #22
Comments
I had that problem too: https://gist.github.com/frodsan/e3547db964c32d9a210f3d2bd58ef2a7 It would be nice to have context information. I don't know if that's easy to do without breaking code, maybe error can be an object? |
I've never run into this, but I think it's mainly because I don't create applications often. I agree with all the points: having the context for some particular assertions looks necessary, but creating an object for each error looks too heavy and changing Scrivener to push an array of assert_length :username, 3..16, [:username, [:not_in_range, 3..16]] To avoid the repetition, a custom assertion can be defined: def assert_in_range(att, range)
assert_length(att, range, [att, [:not_in_range, range]])
end Then: assert_in_range :username, 3..16 We can keep thinking about other ideas. |
@foca I'm thinking again about creating an object. It breaks compatibility, but on the other hand it may be a good approach. It would be ideal if we could create the object only if the validation fails (we could, for example, initialize the error as |
A naive implementation could be: messages = form.errors[field].map do |(code, context)|
ERRORS.fetch(code).call(*context) # note the splat to ignore when `context` is `nil`
end
ERRORS = {
not_present: -> { "can't be empty".freeze },
too_short: ->(val) { "must be at least %d characters".freeze % val },
not_in_range: ->(rng) { "must be between %d and %d".freeze % [rng.first, rng.last] },
# etc
} (This is mostly what I'm doing already) |
Perfect, it looks good to me :-) |
Cool, will draft a PR then :D On Fri, May 20, 2016 at 11:08 PM Michel Martens notifications@github.com
|
Introduce the notion of errors giving some context into why the validation failed. Fixes soveran#22
Something I've run into often is that validation errors can't be fully compressed into a single code. A common enough case is validating length:
If the user's provided username was out of bounds, we'd get a
{ foo: [:not_in_range] }
error. Now, the error message we want to render will most likely need to include the range in which it is valid. Something like "The username must be between 3 and 16 characters long".However, to generate this error, we need to know the range of validity in the view, which means duplicating knowledge. This is worsened when you have multiple front-ends, and the API serialises error codes instead of rendered messages due to i18n).
When the form changes, we need to remember to change the error message in the serialiser and/or probably in the web views if the app also has those.
It would be useful, then, to include context information about the failure in the error itself, so that clients that render this error have all the information from the single source of truth regarding validations: the Scrivener object.
The solution I used (which is probably not be the best, but it worked for me) was to introduce a
Scrivener::Error
object that emulated the error symbol, but added a context attribute. Essentially:And then went and copied scrivener/validations into the project and replaced all the assertions with:
This works, and particularly, kept compatibility with all our existing forms as the error "is still" a Symbol. However, seems a bit heavy handed to wrap an (extra) object around every error code.
While writing this issue, I thought of an alternative, by changing errors into 2-element pairs:
This is pretty minimalistic, and a bit more memory efficient than the above solution, but would break everyone's code if implemented into Scrivener 😄
Is this a "problem" only for me?
We introduced this when a requirement changed and we had to wait for a new release of the mobile application to support these contexts in the API before we were able to implement the change in the business rule itself, which was a bit annoying, as mobile apps aren't particularly fast at upgrading 😛
Is this worth changing in Scrivener itself? Or is everyone fine with duplicating error contexts in wherever you generate error messages / serialise responses to your APIs / whatever.
The text was updated successfully, but these errors were encountered: