Skip to content
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

feat: Implement comparison of matcher #1552

Merged
Merged
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,8 @@ about any of them, make sure to [consult the documentation][rubydocs]!
tests usage of `validates_numericality_of`.
* **[validate_presence_of](lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb)**
tests usage of `validates_presence_of`.
* **[validate_comparison_of](lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb)**
tests usage of `validates_comparison_of`.

### ActiveRecord matchers

Expand Down
3 changes: 2 additions & 1 deletion lib/shoulda/matchers/active_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@
require 'shoulda/matchers/active_model/validate_acceptance_of_matcher'
require 'shoulda/matchers/active_model/validate_confirmation_of_matcher'
require 'shoulda/matchers/active_model/validate_numericality_of_matcher'
require 'shoulda/matchers/active_model/validate_comparison_of_matcher'
require 'shoulda/matchers/active_model/comparison_matcher'
require 'shoulda/matchers/active_model/numericality_matchers/numeric_type_matcher'
require 'shoulda/matchers/active_model/numericality_matchers/comparison_matcher'
require 'shoulda/matchers/active_model/numericality_matchers/odd_number_matcher'
require 'shoulda/matchers/active_model/numericality_matchers/even_number_matcher'
require 'shoulda/matchers/active_model/numericality_matchers/only_integer_matcher'
Expand Down
158 changes: 158 additions & 0 deletions lib/shoulda/matchers/active_model/comparison_matcher.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
require 'active_support/core_ext/module/delegation'

module Shoulda
module Matchers
module ActiveModel
# @private
class ComparisonMatcher < ValidationMatcher
ERROR_MESSAGES = {
:> => {
label: :greater_than,
assertions: [false, false, true],
},
:>= => {
label: :greater_than_or_equal_to,
assertions: [false, true, true],
},
:< => {
label: :less_than,
assertions: [true, false, false],
},
:<= => {
label: :less_than_or_equal_to,
assertions: [true, true, false],
},
:== => {
label: :equal_to,
assertions: [false, true, false],
},
:!= => {
label: :other_than,
assertions: [true, false, true],
},
}.freeze

delegate :failure_message, :failure_message_when_negated, to: :submatchers

def initialize(matcher, value, operator)
super(nil)
unless matcher.respond_to? :diff_to_compare
raise ArgumentError, 'matcher is invalid'
end

@matcher = matcher
@value = value
@operator = operator
@message = ERROR_MESSAGES[operator][:label]
end

def simple_description
description = ''

if expects_strict?
description << ' strictly'
end

description +
"disallow :#{attribute} from being a number that is not " +
"#{comparison_expectation} #{@value}"
end

def for(attribute)
@attribute = attribute
self
end

def with_message(message)
@expects_custom_validation_message = true
@message = message
self
end

def expects_custom_validation_message?
@expects_custom_validation_message
end

def matches?(subject)
@subject = subject
build_option_value
submatchers.matches?(subject)
end

def does_not_match?(subject)
@subject = subject
build_option_value
submatchers.does_not_match?(subject)
end

def comparison_description
"#{comparison_expectation} #{@value}"
end

def submatchers
@_submatchers ||= NumericalityMatchers::Submatchers.new(build_submatchers)
end

private

def build_submatchers
matsales28 marked this conversation as resolved.
Show resolved Hide resolved
comparison_combos.map do |diff, submatcher_method_name|
matcher = __send__(submatcher_method_name, diff, nil)
matcher.with_message(@message, values: { count: @value })
matcher
end
end

def comparison_combos
diffs_to_compare.zip(submatcher_method_names)
end

def submatcher_method_names
assertions.map do |value|
if value
:allow_value_matcher
else
:disallow_value_matcher
end
end
end

def assertions
ERROR_MESSAGES[@operator][:assertions]
end

def build_option_value
@value = case @value
when Proc then @value.call(@subject)
when Symbol then @subject.send(@value)
else @value
end
end
matsales28 marked this conversation as resolved.
Show resolved Hide resolved

def diffs_to_compare
diff_to_compare = @matcher.diff_to_compare
values = case @value
when String then diffs_when_string(diff_to_compare)
else [-1, 0, 1].map { |sign| @value + (diff_to_compare * sign) }
end
matsales28 marked this conversation as resolved.
Show resolved Hide resolved

if @matcher.given_numeric_column?
values
else
values.map(&:to_s)
end
end

def diffs_when_string(diff_to_compare)
[-1, 0, 1].map do |sign|
@value[0..-2] + (@value[-1].ord + diff_to_compare * sign).chr
end
end

def comparison_expectation
ERROR_MESSAGES[@operator][:label].to_s.tr('_', ' ')
end
end
end
end
end

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def submatcher_combos
end

def build_comparison_submatcher(value, operator)
NumericalityMatchers::ComparisonMatcher.new(@numericality_matcher, value, operator).
ComparisonMatcher.new(@numericality_matcher, value, operator).
for(@attribute).
with_message(@message).
on(@context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,46 @@ def matches?(subject)
failing_submatchers.empty?
end

def does_not_match?(subject)
@subject = subject
not_failing_submatchers.empty?
end

def failure_message
last_failing_submatcher.failure_message
failing_submatcher.failure_message
end

def failure_message_when_negated
last_failing_submatcher.failure_message_when_negated
not_failing_submatcher.failure_message_when_negated
end

def add(submatcher)
@submatchers << submatcher
end

def last_failing_submatcher
failing_submatchers.last
end

private

def failing_submatchers
@_failing_submatchers ||= @submatchers.reject do |submatcher|
submatcher.matches?(@subject)
end
end

def not_failing_submatchers
@_not_failing_submatchers ||= @submatchers.reject do |submatcher|
matsales28 marked this conversation as resolved.
Show resolved Hide resolved
submatcher.does_not_match?(@subject)
end
end

def failing_submatcher
failing_submatchers.last
end

def not_failing_submatcher
not_failing_submatchers.detect { |submatcher|
submatcher.instance_of?(DisallowValueMatcher)
} || not_failing_submatchers.first
end
Copy link
Member Author

Choose a reason for hiding this comment

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

That was something that bothered me for a while when creating the first specs for the validate_comparison_of matcher, sometimes the failure message was very hard to understand because it was using AllowValueMatcher failure messages, the DisallowValueMatcher ones are easy to understand when dealing with matcher that didn't fail but was supposed to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... I'm wondering if perhaps we should leave it to the developer to add matchers in the order that would lead to a better user experience instead of trying to "fix" it for them. I can see this behavior being somewhat confusing as well. Is there another approach we could take here or what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tricky because the developer can't define the order of the submatchers. After all, in this specific case, when the user defines one comparison matcher, we define three submatchers for that matcher, and that order is defined by the shoulda-matchers code.

Generates

validates_comparison_of :participant_count, greater_than_or_equal_to: 2
# Generates in shoulda-matchers
# ComparisonMatcher with the following submatchers
# DisallowValueMatcher with value 1
# AllowValueMatcher with value 2
# AllowValue matcher with value 2

I totally get what you're saying about this behavior being confusing. Honestly, I'm fresh out of ideas here. I'm also fine reverting that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted that change on 5906112

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry for not replying, I'm slammed with work currently 😵‍💫 Thank you! If this indeed continues to be confusing, I'm all for revisiting this. It'll also be easier to see the consequences of this change separately.

end
end
end
Expand Down
Loading