diff options
| -rw-r--r-- | app/models/concerns/spammable.rb | 5 | ||||
| -rw-r--r-- | doc/development/README.md | 1 | ||||
| -rw-r--r-- | doc/development/module_with_instance_variables.md | 25 | ||||
| -rw-r--r-- | features/support/env.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/ci/config/entry/configurable.rb | 6 | ||||
| -rw-r--r-- | rubocop/cop/module_with_instance_variables.rb | 6 | ||||
| -rw-r--r-- | spec/rubocop/cop/module_with_instance_variables_spec.rb | 182 |
7 files changed, 108 insertions, 121 deletions
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 8b56894ec3a..5e4274619c4 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -12,6 +12,7 @@ module Spammable attr_accessor :spam attr_accessor :spam_log + alias_method :spam?, :spam after_validation :check_for_spam, on: [:create, :update] @@ -34,10 +35,6 @@ module Spammable end end - def spam? - spam - end - def check_for_spam error_msg = if Gitlab::Recaptcha.enabled? "Your #{spammable_entity_type} has been recognized as spam. "\ diff --git a/doc/development/README.md b/doc/development/README.md index 6892838be7f..c31e665e91a 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -36,6 +36,7 @@ comments: false - [`Gemfile` guidelines](gemfile.md) - [Sidekiq debugging](sidekiq_debugging.md) - [Gotchas](gotchas.md) to avoid +- [Avoid modules with instance variables](module_with_instance_variables.md) if possible - [Issue and merge requests state models](object_state_models.md) - [How to dump production data to staging](db_dump.md) - [Working with the GitHub importer](github_importer.md) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index b663782cd04..43ec8911b81 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -55,10 +55,9 @@ they communicate in a clear way, rather than implicit dependencies. ### Acceptable use -However, it's not all that bad when using instance variables in a module, -as long as it's contained in the same module, that is no other modules or -objects are touching them. If that's the case, then it would be an acceptable -use. +However, it's not always bad to use instance variables in a module, +as long as it's contained in the same module; that is, no other modules or +objects are touching them, then it would be an acceptable use. We especially allow the case where a single instance variable is used with `||=` to setup the value. This would look like: @@ -93,7 +92,7 @@ module Gitlab end ``` -It's still offending because it's not just `||=`, but We could split this +It's still offending because it's not just `||=`, but we could split this method into two: ``` ruby @@ -135,7 +134,7 @@ end ``` There are several implicit dependencies here. First, `params` should be -defined before using. Second, `filter_spam_check_params` should be called +defined before use. Second, `filter_spam_check_params` should be called before `spam_check`. These are all implicit and the includer could be using those instance variables without awareness. @@ -175,18 +174,18 @@ end ``` This way, all those instance variables are isolated in `SpamCheckService` -rather than who ever include the module, and those modules which were also -included, making it much easier to track down the issues if there's any, -and it also reduces the chance of having name conflicts. +rather than whatever includes the module, and those modules which were also +included, making it much easier to track down any issues, +and reducing the chance of having name conflicts. ### Things we might need to ignore right now -Since the way how Rails helpers and mailers work, we might not be able to +Because of the way Rails helpers and mailers work, we might not be able to avoid the use of instance variables there. For those cases, we could ignore them at the moment. At least we're not going to share those modules with -other random objects, so they're still somehow isolated. +other random objects, so they're still somewhat isolated. -### Instance variables in the views +### Instance variables in views They're terrible, because they're also shared between different controllers, and it's very hard to track where those instance variables were set when we @@ -210,5 +209,5 @@ And in the partial: - project = local_assigns.fetch(:project) ``` -This way it's very clear where those values were coming from. In the future, +This way it's clearer where those values were coming from. In the future, we should also forbid the use of instance variables in partials. diff --git a/features/support/env.rb b/features/support/env.rb index b93ddecdced..a1413522f3a 100644 --- a/features/support/env.rb +++ b/features/support/env.rb @@ -42,11 +42,11 @@ module StdoutReporterWithScenarioLocation # Override the standard reporter to show filename and line number next to each # scenario for easy, focused re-runs def before_scenario_run(scenario, step_definitions = nil) - max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? + @max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? # rubocop:disable Cop/ModuleWithInstanceVariables name = scenario.name # This number has no significance, it's just to line things up - max_length = max_step_name_length + 19 + max_length = @max_step_name_length + 19 # rubocop:disable Cop/ModuleWithInstanceVariables out.puts "\n #{'Scenario:'.green} #{name.light_green.ljust(max_length)}" \ " # #{scenario.feature.filename}:#{scenario.line}" end diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index 63b803c15af..db47c2f6185 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -59,13 +59,13 @@ module Gitlab def helpers(*nodes) nodes.each do |symbol| define_method("#{symbol}_defined?") do - @entries[symbol]&.specified? + entries[symbol]&.specified? end define_method("#{symbol}_value") do - return unless @entries[symbol] && @entries[symbol].valid? + return unless entries[symbol] && entries[symbol].valid? - @entries[symbol].value + entries[symbol].value end end end diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb index 974a23bf701..9f9856e308b 100644 --- a/rubocop/cop/module_with_instance_variables.rb +++ b/rubocop/cop/module_with_instance_variables.rb @@ -5,7 +5,7 @@ module RuboCop Do not use instance variables in a module. Please read this for the rationale behind it: - doc/development/module_with_instance_variables.md + https://docs.gitlab.com/ee/development/module_with_instance_variables.html If you think the use for this is fine, please just add: # rubocop:disable Cop/ModuleWithInstanceVariables @@ -56,9 +56,7 @@ module RuboCop add_offense(offense, :expression) end # We allow initialize method and single ivar - elsif initialize_method?(definition) || single_ivar?(definition) - next - else + elsif !initialize_method?(definition) && !single_ivar?(definition) definition.each_descendant(:ivar, :ivasgn) do |offense| add_offense(offense, :expression) end diff --git a/spec/rubocop/cop/module_with_instance_variables_spec.rb b/spec/rubocop/cop/module_with_instance_variables_spec.rb index 72f6b01aa81..df5e2dd2f04 100644 --- a/spec/rubocop/cop/module_with_instance_variables_spec.rb +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -8,7 +8,9 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do subject(:cop) { described_class.new } - shared_examples('registering offense') do + shared_examples('registering offense') do |options| + let(:offending_lines) { options[:offending_lines] } + it 'registers an offense when instance variable is used in a module' do inspect_source(cop, source) @@ -28,63 +30,57 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do end context 'when source is a regular module' do - let(:source) do - <<~RUBY - module M - def f - @f = true + it_behaves_like 'registering offense', offending_lines: [3] do + let(:source) do + <<~RUBY + module M + def f + @f = true + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3] } - - it_behaves_like 'registering offense' end context 'when source is a nested module' do - let(:source) do - <<~RUBY - module N - module M - def f - @f = true + it_behaves_like 'registering offense', offending_lines: [4] do + let(:source) do + <<~RUBY + module N + module M + def f + @f = true + end end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [4] } - - it_behaves_like 'registering offense' end context 'when source is a nested module with multiple offenses' do - let(:source) do - <<~RUBY - module N - module M - def f - @f = true - end - - def g - true - end - - def h - @h = true + it_behaves_like 'registering offense', offending_lines: [4, 12] do + let(:source) do + <<~RUBY + module N + module M + def f + @f = true + end + + def g + true + end + + def h + @h = true + end end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [4, 12] } - - it_behaves_like 'registering offense' end context 'with regular ivar assignment' do @@ -124,78 +120,74 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do end context 'when source is using simple or ivar assignment' do - let(:source) do - <<~RUBY - module M - def f - @f ||= true + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def f + @f ||= true + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is using simple ivar' do - let(:source) do - <<~RUBY - module M - def f? - @f + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def f? + @f + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is defining initialize' do - let(:source) do - <<~RUBY - module M - def initialize - @a = 1 - @b = 2 + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def initialize + @a = 1 + @b = 2 + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is using simple or ivar assignment with other ivar' do - let(:source) do - <<~RUBY - module M - def f - @f ||= g(@g) + it_behaves_like 'registering offense', offending_lines: [3] do + let(:source) do + <<~RUBY + module M + def f + @f ||= g(@g) + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3] } - - it_behaves_like 'registering offense' end context 'when source is using or ivar assignment with something else' do - let(:source) do - <<~RUBY - module M - def f - @f ||= true - @f.to_s + it_behaves_like 'registering offense', offending_lines: [3, 4] do + let(:source) do + <<~RUBY + module M + def f + @f ||= true + @f.to_s + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3, 4] } - - it_behaves_like 'registering offense' end end |