From 367500d9fc3c76aef7add29f48459236928af96e Mon Sep 17 00:00:00 2001 From: Amalrik Maia Date: Fri, 12 Jan 2024 11:14:09 -0300 Subject: [PATCH] make the library work with globally enabled frozen-string-literals (#1598) more refacts to make tests pass with frozen strings fix linter errors clean up some code enable frozen string literals on CI --- .github/workflows/ci.yml | 4 +- .../action_controller/permit_matcher.rb | 14 +- .../set_session_or_flash_matcher.rb | 28 ++-- .../active_model/allow_value_matcher.rb | 2 +- .../active_model/comparison_matcher.rb | 2 +- .../validate_comparison_of_matcher.rb | 14 +- .../validate_numericality_of_matcher.rb | 20 ++- .../validation_matcher/build_description.rb | 13 +- .../association_matchers/optional_matcher.rb | 42 ++--- .../association_matchers/required_matcher.rb | 50 +++--- .../active_record/have_db_index_matcher.rb | 20 +-- .../validate_uniqueness_of_matcher.rb | 152 ++++++++++-------- .../independent/delegate_method_matcher.rb | 26 ++- lib/shoulda/matchers/util.rb | 20 +-- .../unit/helpers/controller_builder.rb | 10 +- 15 files changed, 215 insertions(+), 202 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a91d6672..e5d306f8e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,6 +61,6 @@ jobs: - name: Install dependencies run: bundle install --jobs=3 --retry=3 - name: Run Unit Tests - run: bundle exec rake spec:unit --trace + run: RUBYOPT='--enable=frozen-string-literal' bundle exec rake spec:unit --trace - name: Run Acceptance Tests - run: bundle exec rake spec:acceptance --trace + run: RUBYOPT='--enable=frozen-string-literal' bundle exec rake spec:acceptance --trace diff --git a/lib/shoulda/matchers/action_controller/permit_matcher.rb b/lib/shoulda/matchers/action_controller/permit_matcher.rb index 9e89523bf..449611a6e 100644 --- a/lib/shoulda/matchers/action_controller/permit_matcher.rb +++ b/lib/shoulda/matchers/action_controller/permit_matcher.rb @@ -276,16 +276,14 @@ def failure_message_when_negated :context, :subparameter_name, :parameters_double_registry def expectation - message = 'restrict parameters ' + String.new('restrict parameters ').tap do |message| + if subparameter_name + message << "on #{subparameter_name.inspect} " + end - if subparameter_name - message << "on #{subparameter_name.inspect} " + message << 'to '\ + "#{format_parameter_names(expected_permitted_parameter_names)}" end - - message << 'to '\ - "#{format_parameter_names(expected_permitted_parameter_names)}" - - message end def reality diff --git a/lib/shoulda/matchers/action_controller/set_session_or_flash_matcher.rb b/lib/shoulda/matchers/action_controller/set_session_or_flash_matcher.rb index 4f82ec95b..371ffa052 100644 --- a/lib/shoulda/matchers/action_controller/set_session_or_flash_matcher.rb +++ b/lib/shoulda/matchers/action_controller/set_session_or_flash_matcher.rb @@ -83,25 +83,23 @@ def expected_value_matches? end def expectation_description - string = 'set' - - string << - if key_set? - " #{store.name}[#{key.inspect}]" - else - " any key in #{store.name}" - end - - if expected_value_set? + String.new('set').tap do |string| string << - if expected_value.is_a?(Regexp) - " to a value matching #{expected_value.inspect}" + if key_set? + " #{store.name}[#{key.inspect}]" else - " to #{expected_value.inspect}" + " any key in #{store.name}" end - end - string + if expected_value_set? + string << + if expected_value.is_a?(Regexp) + " to a value matching #{expected_value.inspect}" + else + " to #{expected_value.inspect}" + end + end + end end end end diff --git a/lib/shoulda/matchers/active_model/allow_value_matcher.rb b/lib/shoulda/matchers/active_model/allow_value_matcher.rb index ff33f20cf..660514fa5 100644 --- a/lib/shoulda/matchers/active_model/allow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/allow_value_matcher.rb @@ -550,7 +550,7 @@ def failure_message_preface end def default_failure_message_preface - ''.tap do |preface| + String.new.tap do |preface| if descriptions_for_preset_values.any? preface << 'After setting ' preface << descriptions_for_preset_values.to_sentence diff --git a/lib/shoulda/matchers/active_model/comparison_matcher.rb b/lib/shoulda/matchers/active_model/comparison_matcher.rb index a83ee2ca9..5349b6beb 100644 --- a/lib/shoulda/matchers/active_model/comparison_matcher.rb +++ b/lib/shoulda/matchers/active_model/comparison_matcher.rb @@ -50,7 +50,7 @@ def simple_description description = '' if expects_strict? - description << ' strictly' + description = ' strictly' end description + diff --git a/lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb index b2cec320c..47a673f3a 100644 --- a/lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb @@ -353,16 +353,14 @@ def does_not_match?(subject) end def simple_description - description = '' + String.new.tap do |description| + description << "validate that :#{attribute} looks like " + description << Shoulda::Matchers::Util.a_or_an(allowed_type_name) - description << "validate that :#{attribute} looks like " - description << Shoulda::Matchers::Util.a_or_an(allowed_type_name) - - if comparison_descriptions.present? - description << " #{comparison_descriptions}" + if comparison_descriptions.present? + description << " #{comparison_descriptions}" + end end - - description end def failure_message diff --git a/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb index 0cfd71133..20604af40 100644 --- a/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb @@ -453,20 +453,18 @@ def does_not_match?(subject) end def simple_description - description = '' + String.new.tap do |description| + description << "validate that :#{attribute} looks like " + description << Shoulda::Matchers::Util.a_or_an(full_allowed_type) - description << "validate that :#{attribute} looks like " - description << Shoulda::Matchers::Util.a_or_an(full_allowed_type) - - if range_description.present? - description << " #{range_description}" - end + if range_description.present? + description << " #{range_description}" + end - if comparison_descriptions.present? - description << " #{comparison_descriptions}" + if comparison_descriptions.present? + description << " #{comparison_descriptions}" + end end - - description end def failure_message diff --git a/lib/shoulda/matchers/active_model/validation_matcher/build_description.rb b/lib/shoulda/matchers/active_model/validation_matcher/build_description.rb index 2decbc273..53b897401 100644 --- a/lib/shoulda/matchers/active_model/validation_matcher/build_description.rb +++ b/lib/shoulda/matchers/active_model/validation_matcher/build_description.rb @@ -42,13 +42,12 @@ def description_clauses_for_qualifiers description_clauses = [] if matcher.try(:expects_strict?) - description_clauses << 'raising a validation exception' - - if matcher.try(:expects_custom_validation_message?) - description_clauses.last << ' with a custom message' - end - - description_clauses.last << ' on failure' + description_clauses << + if matcher.try(:expects_custom_validation_message?) + 'raising a validation exception with a custom message on failure' + else + 'raising a validation exception on failure' + end elsif matcher.try(:expects_custom_validation_message?) description_clauses << 'producing a custom validation error on failure' diff --git a/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb index a471f42ee..e4ca4d19f 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb @@ -22,44 +22,48 @@ def matches?(subject) if submatcher_passes?(subject) true else - @missing_option = 'and for the record ' + @missing_option = build_missing_option - missing_option << + false + end + end + + private + + attr_reader :attribute_name, :optional, :submatcher + + def submatcher_passes?(subject) + if optional + submatcher.matches?(subject) + else + submatcher.does_not_match?(subject) + end + end + + def build_missing_option + String.new('and for the record ').tap do |missing_option_string| + missing_option_string << if optional 'not to ' else 'to ' end - missing_option << ( + missing_option_string << ( 'fail validation if '\ ":#{attribute_name} is unset; i.e., either the association "\ 'should have been defined with `optional: '\ "#{optional.inspect}`, or there " ) - missing_option << + missing_option_string << if optional 'should not ' else 'should ' end - missing_option << "be a presence validation on :#{attribute_name}" - - false - end - end - - private - - attr_reader :attribute_name, :optional, :submatcher - - def submatcher_passes?(subject) - if optional - submatcher.matches?(subject) - else - submatcher.does_not_match?(subject) + missing_option_string << "be a presence validation on :#{attribute_name}" end end end diff --git a/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb index ae0536e85..b90e10ef5 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb @@ -23,50 +23,54 @@ def matches?(subject) if submatcher_passes?(subject) true else - @missing_option = 'and for the record ' + @missing_option = build_missing_option - missing_option << + false + end + end + + private + + attr_reader :attribute_name, :required, :submatcher + + def submatcher_passes?(subject) + if required + submatcher.matches?(subject) + else + submatcher.does_not_match?(subject) + end + end + + def validation_message_key + :required + end + + def build_missing_option + String.new('and for the record ').tap do |missing_option_string| + missing_option_string << if required 'to ' else 'not to ' end - missing_option << ( + missing_option_string << ( 'fail validation if '\ ":#{attribute_name} is unset; i.e., either the association "\ 'should have been defined with `required: '\ "#{required.inspect}`, or there " ) - missing_option << + missing_option_string << if required 'should ' else 'should not ' end - missing_option << "be a presence validation on :#{attribute_name}" - - false + missing_option_string << "be a presence validation on :#{attribute_name}" end end - - private - - attr_reader :attribute_name, :required, :submatcher - - def submatcher_passes?(subject) - if required - submatcher.matches?(subject) - else - submatcher.does_not_match?(subject) - end - end - - def validation_message_key - :required - end end end end diff --git a/lib/shoulda/matchers/active_record/have_db_index_matcher.rb b/lib/shoulda/matchers/active_record/have_db_index_matcher.rb index 60bd190d1..7c6dcacb8 100644 --- a/lib/shoulda/matchers/active_record/have_db_index_matcher.rb +++ b/lib/shoulda/matchers/active_record/have_db_index_matcher.rb @@ -152,18 +152,18 @@ def failure_message_when_negated end def description - description = 'have ' - - description << - if qualifiers.include?(:unique) - "#{Shoulda::Matchers::Util.a_or_an(index_type)} " - else - 'an ' - end + String.new('have ').tap do |description| + description << + if qualifiers.include?(:unique) + "#{Shoulda::Matchers::Util.a_or_an(index_type)} " + else + 'an ' + end - description << 'index on ' + description << 'index on ' - description << inspected_expected_columns + description << inspected_expected_columns + end end private diff --git a/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb b/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb index 0426bced6..34dcbdeee 100644 --- a/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb +++ b/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb @@ -411,9 +411,29 @@ def matches_scopes_configuration? if scopes_match? true else - @failure_reason = 'Expected the validation ' + @failure_reason = String.new('Expected the validation ').tap do |failure_reason_string| + failure_reason_string << + if expected_scopes.empty? + 'not to be scoped to anything, ' + else + "to be scoped to #{inspected_expected_scopes}, " + end + + if actual_sets_of_scopes.any? + failure_reason_string << 'but it was scoped to ' + failure_reason_string << "#{inspected_actual_scopes} instead." + else + failure_reason_string << 'but it was not scoped to anything.' + end + end - @failure_reason << + false + end + end + + def build_failure_reason + String.new('Expected the validation ').tap do |failure_reason_string| + failure_reason_string << if expected_scopes.empty? 'not to be scoped to anything, ' else @@ -421,27 +441,25 @@ def matches_scopes_configuration? end if actual_sets_of_scopes.any? - @failure_reason << 'but it was scoped to ' - @failure_reason << "#{inspected_actual_scopes} instead." + failure_reason_string << 'but it was scoped to ' + failure_reason_string << "#{inspected_actual_scopes} instead." else - @failure_reason << 'but it was not scoped to anything.' + failure_reason_string << 'but it was not scoped to anything.' end - - false end end def does_not_match_scopes_configuration? if scopes_match? - @failure_reason = 'Expected the validation ' - - if expected_scopes.empty? - @failure_reason << 'to be scoped to nothing, ' - @failure_reason << 'but it was scoped to ' - @failure_reason << "#{inspected_actual_scopes} instead." - else - @failure_reason << 'not to be scoped to ' - @failure_reason << inspected_expected_scopes + @failure_reason = String.new('Expected the validation ').tap do |failure_reason| + if expected_scopes.empty? + failure_reason << 'to be scoped to nothing, ' + failure_reason << 'but it was scoped to ' + failure_reason << "#{inspected_actual_scopes} instead." + else + failure_reason << 'not to be scoped to ' + failure_reason << inspected_expected_scopes + end end false @@ -618,20 +636,18 @@ def matches_presence_of_scopes? else inspected_scopes = scopes_missing_on_model.map(&:inspect) - reason = '' - - reason << inspected_scopes.to_sentence - - reason << - if inspected_scopes.many? - ' do not seem to be attributes' - else - ' does not seem to be an attribute' - end + @failure_reason = String.new.tap do |reason| + reason << inspected_scopes.to_sentence - reason << " on #{model.name}." + reason << + if inspected_scopes.many? + ' do not seem to be attributes' + else + ' does not seem to be an attribute' + end - @failure_reason = reason + reason << " on #{model.name}." + end false end @@ -643,20 +659,18 @@ def does_not_match_presence_of_scopes? else inspected_scopes = scopes_present_on_model.map(&:inspect) - reason = '' - - reason << inspected_scopes.to_sentence - - reason << - if inspected_scopes.many? - ' seem to be attributes' - else - ' seems to be an attribute' - end + @failure_reason = String.new.tap do |reason| + reason << inspected_scopes.to_sentence - reason << " on #{model.name}." + reason << + if inspected_scopes.many? + ' seem to be attributes' + else + ' seems to be an attribute' + end - @failure_reason = reason + reason << " on #{model.name}." + end false end @@ -936,45 +950,43 @@ def model end def failure_message_preface # rubocop:disable Metrics/MethodLength - prefix = '' - - if @existing_record_created - prefix << "After taking the given #{model.name}" + String.new.tap do |prefix| + if @existing_record_created + prefix << "After taking the given #{model.name}" + + if attribute_setter_for_existing_record + prefix << ', setting ' + prefix << description_for_attribute_setter( + attribute_setter_for_existing_record, + ) + else + prefix << ", whose :#{attribute} is " + prefix << "‹#{existing_value_read.inspect}›" + end - if attribute_setter_for_existing_record - prefix << ', setting ' + prefix << ', and saving it as the existing record, then' + elsif attribute_setter_for_existing_record + prefix << "Given an existing #{model.name}," + prefix << ' after setting ' prefix << description_for_attribute_setter( attribute_setter_for_existing_record, ) + prefix << ', then' else - prefix << ", whose :#{attribute} is " - prefix << "‹#{existing_value_read.inspect}›" + prefix << "Given an existing #{model.name} whose :#{attribute}" + prefix << ' is ' + prefix << Shoulda::Matchers::Util.inspect_value( + existing_value_read, + ) + prefix << ', after' end - prefix << ', and saving it as the existing record, then' - elsif attribute_setter_for_existing_record - prefix << "Given an existing #{model.name}," - prefix << ' after setting ' - prefix << description_for_attribute_setter( - attribute_setter_for_existing_record, - ) - prefix << ', then' - else - prefix << "Given an existing #{model.name} whose :#{attribute}" - prefix << ' is ' - prefix << Shoulda::Matchers::Util.inspect_value( - existing_value_read, - ) - prefix << ', after' - end - - prefix << " making a new #{model.name} and setting " + prefix << " making a new #{model.name} and setting " - prefix << descriptions_for_attribute_setters_for_new_record + prefix << descriptions_for_attribute_setters_for_new_record - prefix << ", the matcher expected the new #{model.name} to be" - - prefix + prefix << ", the matcher expected the new #{model.name} to be" + end end def attribute_changed_value_message diff --git a/lib/shoulda/matchers/independent/delegate_method_matcher.rb b/lib/shoulda/matchers/independent/delegate_method_matcher.rb index d7b88dc86..defd5f2b8 100644 --- a/lib/shoulda/matchers/independent/delegate_method_matcher.rb +++ b/lib/shoulda/matchers/independent/delegate_method_matcher.rb @@ -440,22 +440,20 @@ def calls_on_delegate_object end def formatted_calls_on_delegate_object - string = '' - - if calls_on_delegate_object.any? - string << "\n\n" - calls_on_delegate_object.each_with_index do |call, i| - name = call.method_name - args = call.args.map(&:inspect).join(', ') - string << "#{i + 1}) #{name}(#{args})\n" + String.new.tap do |string| + if calls_on_delegate_object.any? + string << "\n\n" + calls_on_delegate_object.each_with_index do |call, i| + name = call.method_name + args = call.args.map(&:inspect).join(', ') + string << "#{i + 1}) #{name}(#{args})\n" + end + else + string << ' (none)' end - else - string << ' (none)' - end - string.rstrip! - - string + string.rstrip! + end end end end diff --git a/lib/shoulda/matchers/util.rb b/lib/shoulda/matchers/util.rb index 7813860d5..521446325 100644 --- a/lib/shoulda/matchers/util.rb +++ b/lib/shoulda/matchers/util.rb @@ -70,17 +70,17 @@ def self.inspect_range(range) end def self.inspect_hash(hash) - output = '‹{' + String.new('‹{').tap do |output| + output << hash.map { |key, value| + if key.is_a?(Symbol) + "#{key}: #{value.inspect}" + else + "#{key.inspect} => #{value.inspect}" + end + }.join(', ') - output << hash.map { |key, value| - if key.is_a?(Symbol) - "#{key}: #{value.inspect}" - else - "#{key.inspect} => #{value.inspect}" - end - }.join(', ') - - output << '}›' + output << '}›' + end end def self.dummy_value_for(column_type, array: false) diff --git a/spec/support/unit/helpers/controller_builder.rb b/spec/support/unit/helpers/controller_builder.rb index 544b38ae0..1fba38804 100644 --- a/spec/support/unit/helpers/controller_builder.rb +++ b/spec/support/unit/helpers/controller_builder.rb @@ -10,9 +10,13 @@ def self.configure_example_group(example_group) end def define_controller(class_name, &block) - class_name = class_name.to_s - class_name << 'Controller' unless class_name =~ /Controller$/ - define_class(class_name, ActionController::Base, &block) + new_class_name = if class_name.to_s =~ /Controller$/ + class_name.to_s + else + "#{class_name}Controller" + end + + define_class(new_class_name, ActionController::Base, &block) end def define_routes(&block)