Add in: range matcher to validate_numericality_of
#1512
Conversation
Closes: thoughtbot#1493 In Rails 7 was added a new option to validate numericality. You can use `in: range` to specify a range to validate an attribute. ```ruby class User < ApplicationRecord validates :age, numericality: { greater_than_or_equal_to: 18, less_than_or_equal_to: 65 } end class User < ApplicationRecord validates :age, numericality: { in: 18..65 } end ``` In this commit we are adding the support matcher to this new functionality, while also making a refactor on the numericality matchers that use the concept of submatchers. We've created a new class (`NumericalityMatchers::Submatcher`) that's been used by `NumericalityMatchers::RangeMatcher` and `NumericalityMatchers::ComparisonMatcher`, this new class wil handle shared logic regarding having submatchers that will check if the parent matcher is valid or not. Our new class `Numericality::Matchers::RangeMatcher` is using as submatchers two `NumericalityMatchers::ComparisonMatcher` instances to avoid creating new logic to handle this new option and also to replicate what was being used before this option existed in Rails (see example above) In this commit we are adding: * NumericalityMatchers::RangeMatcher file to support the new `in: range` option. * Specs on ValidateNumericalityOfMatcherSpec file for the new supported option, only running on rails_versions > 7. * NumericalityMatchers::Submatchers file to handle having submatchers inside a matcher file. * Refactors to NumericalityMatchers::ComparisonMatcher.
matsales28
force-pushed
the
feat/validate_numericality_of-in-matcher
branch
from
9699c78 to
14379aa
Compare
| hash[qualifier[:name]] = qualifier[:validation_name] | ||
| def all_available_qualifiers | ||
| all_qualifiers.filter do |qualifier| | ||
| rails_version >= qualifier.fetch(:rails_version, rails_oldest_version_supported) | ||
| end | ||
| end |
There was a problem hiding this comment.
This method was not being used at all, I've deleted it, but happy to revert if it's needed in some way or not in the scope of this PR to make this change.
| if args | ||
| matcher.__send__(qualifier[:name], args) | ||
| else | ||
| matcher.__send__(qualifier[:name]) | ||
| end |
There was a problem hiding this comment.
This method was causing an error because args when testing the in option was a range, and the splat operator was destructuring the range.
We were using the splat operator on args only to prevent passing it when it was nil, so I changed this method to reflect that better and not cause the error when using a range.
Happy to change to this other implementation if you think the use case of not destructuring is more clear when the argument is a range.
| if args | |
| matcher.__send__(qualifier[:name], args) | |
| else | |
| matcher.__send__(qualifier[:name]) | |
| end | |
| if args.is_a? Range | |
| matcher.__send__(qualifier[:name], args) | |
| else | |
| matcher.__send__(qualifier[:name], *args) | |
| end |
|
Happy to contribute to a gem that I used a lot. It was pretty challenging but a fun journey to make this PR because I didn't know the codebase at all. I've tried to make a minor refactoring. If y'all think that's not in the scope of this PR, I'm happy to revert the changes and limit my PR only to add the new functionality, and any suggestions to make the code clearer / better are welcome! 😄 |
|
Hey! I skimmed over the PR. My initial thought is that I feel like I've done something similar in years past, where I've created an abstraction around a group of matchers rather than just one, but this particular implementation is really straightforward, so I like it. I'll give this a deeper dive today. |
|
@mcmire How is the review of this PR going? Does it look like it will be accepted into the shoulda-matchers codebase? No worries if you've been busy on other projects and haven't had time to look at it. |
|
I'm on vacation right now but honestly I'm fine with merging this PR as well. If there's duplication we can always refactor it out later, it's no big deal. |
@mcmire thanks for the update, hope you enjoy the rest of your vacation :) |
Closes: #1493
In Rails 7 was added a new option to validate numericality. You can use
in: rangeto specify a range to validate an attribute.In this commit we are adding the support matcher to this new functionality, while also making a refactor on the numericality matchers that use the concept of submatchers.
We've created a new class (
NumericalityMatchers::Submatcher) that's been used byNumericalityMatchers::RangeMatcherandNumericalityMatchers::ComparisonMatcher, this new class wil handle shared logic regarding having submatchers that will check if the parent matcher is valid or not.Our new class
Numericality::Matchers::RangeMatcheris using as submatchers twoNumericalityMatchers::ComparisonMatcherinstances to avoid creating new logic to handle this new option and also to replicate what was being used before this option existed in Rails (see example above)In this commit we are adding:
in: rangeoption.