-
-
Notifications
You must be signed in to change notification settings - Fork 910
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
Extend AM validate_length_of matcher to support array attributes #1560
Extend AM validate_length_of matcher to support array attributes #1560
Conversation
…force validation as_array
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.
These changes make sense to me! Nice work. Just had a few comments.
lib/shoulda/matchers/active_model/validate_absence_of_matcher.rb
Outdated
Show resolved
Hide resolved
lib/shoulda/matchers/active_model/validate_length_of_matcher.rb
Outdated
Show resolved
Hide resolved
spec/unit/shoulda/matchers/active_model/validate_length_of_matcher_spec.rb
Show resolved
Hide resolved
@mcmire changes applied. This one is ready for another iteration. 👍 |
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.
Looking better! I just had two more suggestions related to documentation.
lib/shoulda/matchers/active_model/validate_length_of_matcher.rb
Outdated
Show resolved
Hide resolved
lib/shoulda/matchers/active_model/validate_length_of_matcher.rb
Outdated
Show resolved
Hide resolved
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.
Looks good! Thanks so much for the PR!
First step to close: #1559
Solution
Were added two mechanisms to treat
validate_length_of_matcher
related attribute as array. One was througharray_column?
helper(also used onvalidate_absence_of_matcher
) based on column type ifarray?
helper is defined on model'scolumns_hash
and another one with an explicit chain methodas_array
that will force to validate column with arrays values.Why this way? First one will cover pure
array: true
columns for postgresql, second one could be dynamically used to check, for example, json columns storing arrays and being treated like that. This second approach will leave more freedom when using the validator.Main validate_length_of_matcher specs were duplicated for the two variants. After all review's iteration for this PR concluded and if it's finally approved and merged this approach, I'll continue extending the array support to other validators(as inclusion/exclusion).