-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
Add slice! method to ActiveModel::Errors #34489
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
# person.errors.slice!(:age, :gender) # => { :age=>["cannot be nil"], :gender=>["cannot be nil"]} | ||
# person.errors.keys(:name, :age) # => [:name] | ||
def slice!(*keys) | ||
keys.inject({}) do |acc, key| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The messages
and details
are both hashes. I think it makes more sense for us to just use slice!
on those like we do with merge!
, right?
person.errors.add(:gender, "cannot be nil") | ||
person.errors.add(:city, "cannot be nil") | ||
|
||
removed_errors = person.errors.slice!(:age, :gender) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a test with string keys, since all other methods support both strings and symbols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
Should I mix strings and symbols in this test or is it better to create another test just for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can mix them if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I understand! This method should allow strings or symbols to be passed indifferently. I have a failing test now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Can you add a CHANGELOG entry and also squash all commits in one?
Done! Tests are passing now. |
Thank you so much! Nice work |
Add slice! method to ActiveModel::Errors rails/rails#34489
Summary
ActiveModel::Errors
implements several methods fromHash
but not all of them.The method
slice!
is provided toHash
viaActiveSuport
and it could be really convenient to have it as well inActiveModel::Errors
for situations when the developer needs to delete several errors from a model.