Custom error messages for required properties in Hashie::Dash subclasses #232
Conversation
|
|
||
| required_option = options.delete(:required) | ||
| if required_option | ||
| fail(KeyError, 'Only :message key supported.') if required_option.is_a?(::Hash) && !required_option[:message] |
There was a problem hiding this comment.
Shouldn't this be an ArgumentError. Also maybe future-proof it by comparing .keys instead of reaching into :message.
|
I like it. See my comment above, otherwise good to merge. |
|
What do you think about this change: fail(ArgumentError, 'Only :message key supported.') if required_option.is_a?(::Hash) && !required_option.keys.eql?([:message])If you approve, then I can change specs and update PR. |
|
Good. |
|
|
||
| return option if option.is_a?(String) | ||
| return option[self] if option.is_a?(Proc) | ||
| return option[:message] if option.is_a?(::Hash) |
There was a problem hiding this comment.
I'm not a fan of all of this type-checking, though it's pretty prevalent throughout the codebase. Is the extra flexibility of allowing Strings, Procs, and Hashes as the required option worth decreasing the readability of the code that handles it?
Personally, I like the original suggested implementation in #98.
class Credentials < Hashie::Dash
property :req_attr, :required => true, :message => "is required for the credentials hash"
endThis is pretty clear and wouldn't require all of the type-checking, which would make the code more readable.
|
I think i have to agree with @michaelherold, it seems like not nesting anything inside |
No description provided.