Skip to content

Conversation

@tomeon
Copy link

@tomeon tomeon commented Dec 8, 2015

I hesitate to make this PR given that part of Values' appeal is its exceptionally small size; so, apologies in advance if the changeset it unwanted, and thanks for taking the time to review it.

I use Values for input validation and would find it helpful if the exceptions raised by the .new and .with methods upon receipt of bad arguments contained lists of missing and unrecognized keys, along the same lines as how virtus's CoercionError exception class has the .attribute_name method for helping to determine which failed attribute prevented an object's instantiation.

Since Values already validates constructor arguments, this PR mostly amounts to percolating existing data up to the caller -- the exception being the second argument to the Values::ArgumentError created in .new.

The convention I've followed is that the @missing_keys and @unexpected_keys attributes will contain empty arrays if there were, respectively, no missing arguments or no extra arguments to .with. By contrast, @unexpected_keys will be nil if there were extra arguments provided to .new, since there is no way to determine the 'meaning' of additional positional parameters in the absence of handy hash keys acting a labels.

@tom-pang
Copy link
Owner

tom-pang commented Dec 8, 2015

@BaxterStockman whilst I agree on Values' small size, I like this PR and would like to merge it. I have some nitpicks that I'd like addressed first.

lib/values.rb Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please to rename to FieldError

1. {missing,unexpected}_keys => {missing,unexpected}_values
2. Values::ArgumentError => Values::FieldError
3. Default values for @{missing,unexpected}_fields now [] instead of nil
@tomeon
Copy link
Author

tomeon commented Dec 9, 2015

@tcrayford -- many thanks for your feedback! I've updated the PR in response to your comments.

@ms-ati
Copy link
Contributor

ms-ati commented Dec 28, 2015

@tcrayford -- are you planning to merge this and release a 1.10.0?

@ms-ati
Copy link
Contributor

ms-ati commented Apr 5, 2016

ping

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