diff options
61 files changed, 599 insertions, 48 deletions
diff --git a/app/controllers/concerns/boards_responses.rb b/app/controllers/concerns/boards_responses.rb index 2c9c095a5d7..058e4591770 100644 --- a/app/controllers/concerns/boards_responses.rb +++ b/app/controllers/concerns/boards_responses.rb @@ -23,10 +23,12 @@ module BoardsResponses return render_403 unless can?(current_user, ability, resource) end + # rubocop:disable Cop/ModuleWithInstanceVariables def respond_with_boards respond_with(@boards) end + # rubocop:disable Cop/ModuleWithInstanceVariables def respond_with_board respond_with(@board) end diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 782f0be9c4a..0350c9228c9 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -1,6 +1,7 @@ module CreatesCommit extend ActiveSupport::Concern + # rubocop:disable Cop/ModuleWithInstanceVariables def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) if can?(current_user, :push_code, @project) @project_to_commit_into = @project @@ -77,6 +78,7 @@ module CreatesCommit end end + # rubocop:disable Cop/ModuleWithInstanceVariables def new_merge_request_path project_new_merge_request_path( @project_to_commit_into, @@ -93,6 +95,7 @@ module CreatesCommit project_merge_request_path(@project, @merge_request) end + # rubocop:disable Cop/ModuleWithInstanceVariables def merge_request_exists? return @merge_request if defined?(@merge_request) @@ -100,10 +103,12 @@ module CreatesCommit .find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch) end + # rubocop:disable Cop/ModuleWithInstanceVariables def different_project? @project_to_commit_into != @project end + # rubocop:disable Cop/ModuleWithInstanceVariables def create_merge_request? # Even if the field is set, if we're checking the same branch # as the target branch in the same project, diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 9c222549cdc..01645d4f6c1 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -19,7 +19,7 @@ module IssuableActions end def update - @issuable = update_service.execute(issuable) + @issuable = update_service.execute(issuable) # rubocop:disable Cop/ModuleWithInstanceVariables respond_to do |format| format.html do @@ -85,7 +85,7 @@ module IssuableActions def render_conflict_response respond_to do |format| format.html do - @conflict = true + @conflict = true # rubocop:disable Cop/ModuleWithInstanceVariables render :edit end @@ -99,6 +99,7 @@ module IssuableActions end end + # rubocop:disable Cop/ModuleWithInstanceVariables def labels @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute end @@ -109,6 +110,7 @@ module IssuableActions end end + # rubocop:disable Cop/ModuleWithInstanceVariables def authorize_admin_issuable! unless can?(current_user, :"admin_#{resource_name}", @project) return access_denied! diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 3181f517087..cfd1d077fe8 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -10,6 +10,7 @@ module IssuableCollections private + # rubocop:disable Cop/ModuleWithInstanceVariables def set_issues_index @collection_type = "Issue" @issues = issues_collection @@ -84,6 +85,7 @@ module IssuableCollections finder_class.new(current_user, filter_params) end + # rubocop:disable Cop/ModuleWithInstanceVariables def filter_params set_sort_order_from_cookie set_default_state diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb index 404559c8707..a28dd376c80 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/app/controllers/concerns/issues_action.rb @@ -2,6 +2,7 @@ module IssuesAction extend ActiveSupport::Concern include IssuableCollections + # rubocop:disable Cop/ModuleWithInstanceVariables def issues @label = issues_finder.labels.first diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb index d3c8e4888bc..0f61e1ccc06 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/app/controllers/concerns/merge_requests_action.rb @@ -2,6 +2,7 @@ module MergeRequestsAction extend ActiveSupport::Concern include IssuableCollections + # rubocop:disable Cop/ModuleWithInstanceVariables def merge_requests @label = merge_requests_finder.labels.first diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index 081f3336780..46facd915c8 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module MilestoneActions extend ActiveSupport::Concern diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 57b45f335fa..be6062c7d55 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module NotesActions include RendersNotes extend ActiveSupport::Concern diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index 9849aa93fa6..f0a68f23566 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -14,6 +14,6 @@ module OauthApplications end def load_scopes - @scopes = Doorkeeper.configuration.scopes + @scopes ||= Doorkeeper.configuration.scopes end end diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb index bb2c1dfa00a..675fefd0d36 100644 --- a/app/controllers/concerns/renders_commits.rb +++ b/app/controllers/concerns/renders_commits.rb @@ -1,4 +1,5 @@ module RendersCommits + # rubocop:disable Cop/ModuleWithInstanceVariables def prepare_commits_for_rendering(commits) Banzai::CommitRenderer.render(commits, @project, current_user) diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb index 4791bc561a4..4185396e24b 100644 --- a/app/controllers/concerns/renders_notes.rb +++ b/app/controllers/concerns/renders_notes.rb @@ -1,4 +1,5 @@ module RendersNotes + # rubocop:disable Cop/ModuleWithInstanceVariables def prepare_notes_for_rendering(notes, noteable = nil) preload_noteable_for_regular_notes(notes) preload_max_access_for_authors(notes, @project) diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index be2e6c7f193..ce60267f345 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -65,6 +65,7 @@ module ServiceParams # Parameters to ignore if no value is specified FILTER_BLANK_PARAMS = [:password].freeze + # rubocop:disable Cop/ModuleWithInstanceVariables def service_params dynamic_params = @service.event_channel_names + @service.event_names service_params = params.permit(:id, service: ALLOWED_PARAMS_CE + dynamic_params) diff --git a/app/controllers/concerns/snippets_actions.rb b/app/controllers/concerns/snippets_actions.rb index ffea712a833..4216c1fe063 100644 --- a/app/controllers/concerns/snippets_actions.rb +++ b/app/controllers/concerns/snippets_actions.rb @@ -4,6 +4,7 @@ module SnippetsActions def edit end + # rubocop:disable Cop/ModuleWithInstanceVariables def raw disposition = params[:inline] == 'false' ? 'attachment' : 'inline' diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index ada0dde87fb..ef6e14c9e4c 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -17,6 +17,7 @@ module SpammableActions private + # rubocop:disable Cop/ModuleWithInstanceVariables def ensure_spam_config_loaded! return @spam_config_loaded if defined?(@spam_config_loaded) diff --git a/app/controllers/concerns/toggle_subscription_action.rb b/app/controllers/concerns/toggle_subscription_action.rb index 92cb534343e..0a6d40d36ea 100644 --- a/app/controllers/concerns/toggle_subscription_action.rb +++ b/app/controllers/concerns/toggle_subscription_action.rb @@ -11,6 +11,7 @@ module ToggleSubscriptionAction private + # rubocop:disable Cop/ModuleWithInstanceVariables def subscribable_project @project || raise(NotImplementedError) end diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 1db6b2d2fa2..7644f2ea95f 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -43,6 +43,7 @@ module Mentionable self end + # rubocop:disable Cop/ModuleWithInstanceVariables def all_references(current_user = nil, extractor: nil) @extractors ||= {} diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 710fc1ed647..0f506e6aa25 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -94,6 +94,7 @@ module Milestoneish end end + # rubocop:disable Cop/ModuleWithInstanceVariables def memoize_per_user(user, method_name) @memoized ||= {} @memoized[method_name] ||= {} diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 5d75b2aa6a3..b44274f6145 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -46,6 +46,7 @@ module Noteable notes.inc_relations_for_view.grouped_diff_discussions(*args) end + # rubocop:disable Cop/ModuleWithInstanceVariables def resolvable_discussions @resolvable_discussions ||= if defined?(@discussions) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index ce69fd34ac5..e48bc0be410 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -56,15 +56,17 @@ module Participable # # Returns an Array of User instances. def participants(current_user = nil) - @participants ||= Hash.new do |hash, user| - hash[user] = raw_participants(user) - end - - @participants[current_user] + all_participants[current_user] end private + def all_participants + @all_participants ||= Hash.new do |hash, user| + hash[user] = raw_participants(user) + end + end + def raw_participants(current_user = nil) current_user ||= author ext = Gitlab::ReferenceExtractor.new(project, current_user) diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index e961c97e337..9a7b9cd12b0 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -44,6 +44,7 @@ module RelativePositioning next_pos end + # rubocop:disable Cop/ModuleWithInstanceVariables def move_between(before, after) return move_after(before) unless after return move_before(after) unless before @@ -58,6 +59,7 @@ module RelativePositioning self.relative_position = position_between(before.relative_position, after.relative_position) end + # rubocop:disable Cop/ModuleWithInstanceVariables def move_after(before = self) pos_before = before.relative_position pos_after = before.next_relative_position @@ -73,6 +75,7 @@ module RelativePositioning self.relative_position = position_between(pos_before, pos_after) end + # rubocop:disable Cop/ModuleWithInstanceVariables def move_before(after = self) pos_after = after.relative_position pos_before = after.prev_relative_position @@ -132,6 +135,7 @@ module RelativePositioning end end + # rubocop:disable Cop/ModuleWithInstanceVariables def save_positionable_neighbours return unless @positionable_neighbours diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index f006a271327..56ba4a9a4d0 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -30,12 +30,14 @@ module ResolvableDiscussion allow_nil: true end + # rubocop:disable Cop/ModuleWithInstanceVariables def resolvable? return @resolvable if @resolvable.present? @resolvable = potentially_resolvable? && notes.any?(&:resolvable?) end + # rubocop:disable Cop/ModuleWithInstanceVariables def resolved? return @resolved if @resolved.present? @@ -46,12 +48,14 @@ module ResolvableDiscussion @first_note ||= notes.first end + # rubocop:disable Cop/ModuleWithInstanceVariables def first_note_to_resolve return unless resolvable? @first_note_to_resolve ||= notes.find(&:to_be_resolved?) end + # rubocop:disable Cop/ModuleWithInstanceVariables def last_resolved_note return unless resolved? @@ -88,6 +92,7 @@ module ResolvableDiscussion private + # rubocop:disable Cop/ModuleWithInstanceVariables def update # Do not select `Note.resolvable`, so that system notes remain in the collection notes_relation = Note.where(id: notes.map(&:id)) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 22fde2eb134..05ddae42d2d 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -86,6 +86,7 @@ module Routable end end + # rubocop:disable Cop/ModuleWithInstanceVariables def full_name if route && route.name.present? @full_name ||= route.name @@ -125,6 +126,7 @@ module Routable private + # rubocop:disable Cop/ModuleWithInstanceVariables def uncached_full_path if route && route.path.present? @full_path ||= route.path @@ -162,6 +164,7 @@ module Routable route.save end + # rubocop:disable Cop/ModuleWithInstanceVariables def prepare_route route || build_route(source: self) route.path = build_full_path diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 731d9b9a745..8b56894ec3a 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -35,7 +35,7 @@ module Spammable end def spam? - @spam + spam end def check_for_spam diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index 25e2d8ea24e..a73de49b9bb 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -36,6 +36,7 @@ module Taskable end # Called by `TaskList::Summary` + # rubocop:disable Cop/ModuleWithInstanceVariables def task_list_items return [] if description.blank? diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index 9f403d96ed5..49438908c36 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -4,7 +4,6 @@ # # Used by Issue and MergeRequest. # - module TimeTrackable extend ActiveSupport::Concern @@ -21,6 +20,7 @@ module TimeTrackable has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent end + # rubocop:disable Cop/ModuleWithInstanceVariables def spend_time(options) @time_spent = options[:duration] @time_spent_user = options[:user] @@ -51,6 +51,7 @@ module TimeTrackable private + # rubocop:disable Cop/ModuleWithInstanceVariables def reset_spent_time timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) end @@ -63,6 +64,7 @@ module TimeTrackable ) end + # rubocop:disable Cop/ModuleWithInstanceVariables def check_negative_time_spent return if time_spent.nil? || time_spent == :reset diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb index 7d45b4aa26a..df78ffbb6bd 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -2,11 +2,13 @@ module Issues module ResolveDiscussions attr_reader :merge_request_to_resolve_discussions_of_iid, :discussion_to_resolve_id + # rubocop:disable Cop/ModuleWithInstanceVariables def filter_resolve_discussion_params @merge_request_to_resolve_discussions_of_iid ||= params.delete(:merge_request_to_resolve_discussions_of) @discussion_to_resolve_id ||= params.delete(:discussion_to_resolve) end + # rubocop:disable Cop/ModuleWithInstanceVariables def merge_request_to_resolve_discussions_of return @merge_request_to_resolve_discussions_of if defined?(@merge_request_to_resolve_discussions_of) @@ -15,6 +17,7 @@ module Issues .find_by(iid: merge_request_to_resolve_discussions_of_iid) end + # rubocop:disable Cop/ModuleWithInstanceVariables def discussions_to_resolve return [] unless merge_request_to_resolve_discussions_of diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 11030bee8f1..9b2a601f84b 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -7,6 +7,7 @@ # - params with :request # module SpamCheckService + # rubocop:disable Cop/ModuleWithInstanceVariables def filter_spam_check_params @request = params.delete(:request) @api = params.delete(:api) @@ -17,6 +18,7 @@ module SpamCheckService # In order to be proceed to the spam check process, @spammable has to be # a dirty instance, which means it should be already assigned with the new # attribute values. + # rubocop:disable Cop/ModuleWithInstanceVariables def spam_check(spammable, user) spam_service = SpamService.new(spammable, @request) diff --git a/app/workers/concerns/new_issuable.rb b/app/workers/concerns/new_issuable.rb index eb0d6c9c36c..b6aea921aae 100644 --- a/app/workers/concerns/new_issuable.rb +++ b/app/workers/concerns/new_issuable.rb @@ -8,12 +8,14 @@ module NewIssuable user && issuable end + # rubocop:disable Cop/ModuleWithInstanceVariables def set_user(user_id) @user = User.find_by(id: user_id) log_error(User, user_id) unless @user end + # rubocop:disable Cop/ModuleWithInstanceVariables def set_issuable(issuable_id) @issuable = issuable_class.find_by(id: issuable_id) diff --git a/config/initializers/fix_local_cache_middleware.rb b/config/initializers/fix_local_cache_middleware.rb index cb37f9ed22c..c9abe0c1255 100644 --- a/config/initializers/fix_local_cache_middleware.rb +++ b/config/initializers/fix_local_cache_middleware.rb @@ -4,6 +4,7 @@ module LocalCacheRegistryCleanupWithEnsure LocalStore = ActiveSupport::Cache::Strategy::LocalCache::LocalStore + # rubocop:disable Cop/ModuleWithInstanceVariables def call(env) LocalCacheRegistry.set_cache_for(local_cache_key, LocalStore.new) response = @app.call(env) diff --git a/config/initializers/rspec_profiling.rb b/config/initializers/rspec_profiling.rb index 16b9d5b15e5..d6c75a611b9 100644 --- a/config/initializers/rspec_profiling.rb +++ b/config/initializers/rspec_profiling.rb @@ -16,6 +16,7 @@ module RspecProfilingExt end module Run + # rubocop:disable Cop/ModuleWithInstanceVariables def example_finished(*args) super rescue => err diff --git a/config/initializers/rugged_use_gitlab_git_attributes.rb b/config/initializers/rugged_use_gitlab_git_attributes.rb index 7d652799786..abfa2c354cd 100644 --- a/config/initializers/rugged_use_gitlab_git_attributes.rb +++ b/config/initializers/rugged_use_gitlab_git_attributes.rb @@ -14,6 +14,7 @@ module Rugged class Repository module UseGitlabGitAttributes + # rubocop:disable Cop/ModuleWithInstanceVariables def fetch_attributes(name, *) @attributes ||= Gitlab::Git::Attributes.new(path) @attributes.attributes(name) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md new file mode 100644 index 00000000000..b663782cd04 --- /dev/null +++ b/doc/development/module_with_instance_variables.md @@ -0,0 +1,214 @@ +## Modules with instance variables could be considered harmful + +### Background + +Rails somehow encourages people using modules and instance variables +everywhere. For example, using instance variables in the controllers, +helpers, and views. They're also encouraging the use of +`ActiveSupport::Concern`, which further strengthens the idea of +saving everything in a giant, single object, and people could access +everything in that one giant object. + +### The problems + +Of course this is convenient to develop, because we just have everything +within reach. However this has a number of downsides when that chosen object +is growing, it would later become out of control for the same reason. + +There are just too many things in the same context, and we don't know if +those things are tightly coupled or not, depending on each others or not. +It's very hard to tell when the complexity grows to a point, and it makes +tracking the code also extremely hard. For example, a class could be using +3 different instance variables, and all of them could be initialized and +manipulated from 3 different modules. It's hard to track when those variables +start giving us troubles. We don't know which module would suddenly change +one of the variables. Everything could touch anything. + +### Similar concerns + +People are saying multiple inheritance is bad. Mixing multiple modules with +multiple instance variables scattering everywhere suffer from the same issue. +The same applies to `ActiveSupport::Concern`. See: +[Consider replacing concerns with dedicated classes & composition]( +https://gitlab.com/gitlab-org/gitlab-ce/issues/23786) + +There's also a similar idea: +[Use decorators and interface segregation to solve overgrowing models problem]( +https://gitlab.com/gitlab-org/gitlab-ce/issues/13484) + +Note that `included` doesn't solve the whole issue. They define the +dependencies, but they still allow each modules to talk implicitly via the +instance variables in the final giant object, and that's where the problem is. + +### Solutions + +We should split the giant object into multiple objects, and they communicate +with each other with the API, i.e. public methods. In short, composition over +inheritance. This way, each smaller objects would have their own respective +limited states, i.e. instance variables. If one instance variable goes wrong, +we would be very clear that it's from that single small object, because +no one else could be touching it. + +With clearly defined API, this would make things less coupled and much easier +to debug and track, and much more extensible for other objects to use, because +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. + +We especially allow the case where a single instance variable is used with +`||=` to setup the value. This would look like: + +``` ruby +module M + def f + @f ||= true + end +end +``` + +Unfortunately it's not easy to code more complex rules into the cop, so +we rely on people's best judgement. If we could find another good pattern +we could easily add to the cop, we should do it. + +### How to rewrite and avoid disabling this cop + +Even if we could just disable the cop, we should avoid doing so. Some code +could be easily rewritten in simple form. Here's an example. Consider this +acceptable method: + +``` ruby +module Gitlab + module Emoji + def emoji_unicode_version(name) + @emoji_unicode_versions_by_name ||= + JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json'))) + @emoji_unicode_versions_by_name[name] + end + end +end +``` + +It's still offending because it's not just `||=`, but We could split this +method into two: + +``` ruby +module Gitlab + module Emoji + def emoji_unicode_version(name) + emoji_unicode_versions_by_name[name] + end + + private + + def emoji_unicode_versions_by_name + @emoji_unicode_versions_by_name ||= + JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json'))) + end + end +end +``` + +Now the cop won't complain. Here's another bad example which we could rewrite: + +``` ruby +module SpamCheckService + def filter_spam_check_params + @request = params.delete(:request) + @api = params.delete(:api) + @recaptcha_verified = params.delete(:recaptcha_verified) + @spam_log_id = params.delete(:spam_log_id) + end + + def spam_check(spammable, user) + spam_service = SpamService.new(spammable, @request) + + spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do + user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) + end + end +end +``` + +There are several implicit dependencies here. First, `params` should be +defined before using. 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. + +This should be rewritten like: + +``` ruby +class SpamCheckService + def initialize(request:, api:, recaptcha_verified:, spam_log_id:) + @request = request + @api = api + @recaptcha_verified = recaptcha_verified + @spam_log_id = spam_log_id + end + + def spam_check(spammable, user) + spam_service = SpamService.new(spammable, @request) + + spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do + user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) + end + end +end +``` + +And use it like: + +``` ruby +class UpdateSnippetService < BaseService + def execute + # ... + spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id)) + + spam.check(snippet, current_user) + # ... + end +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. + +### Things we might need to ignore right now + +Since the way how 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. + +### Instance variables in the 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 +saw somewhere is using it, neither do we know where those were used when we +saw somewhere is setting up them. We hit into a number of 500 errors when we +tried to remove some instance variables in the controller in the past. + +Somewhere, some partials might be using it, and we don't know. + +We're trying to use something like this instead: + +``` haml += render 'projects/commits/commit', commit: commit, ref: ref, project: project +``` + +And in the partial: + +``` haml +- ref = local_assigns.fetch(:ref) +- commit = local_assigns.fetch(:commit) +- project = local_assigns.fetch(:project) +``` + +This way it's very clear 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 5962745d501..b93ddecdced 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? 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 out.puts "\n #{'Scenario:'.green} #{name.light_green.ljust(max_length)}" \ " # #{scenario.feature.filename}:#{scenario.line}" end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 1c12166e434..d6df269486a 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -32,6 +32,7 @@ module API end end + # rubocop:disable Cop/ModuleWithInstanceVariables def current_user return @current_user if defined?(@current_user) @@ -393,6 +394,7 @@ module API end end + # rubocop:disable Cop/ModuleWithInstanceVariables def sudo! return unless sudo_identifier diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 4c0db4d42b1..6bb85dd2619 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -6,20 +6,20 @@ module API 'git-upload-pack' => :ssh_upload_pack }.freeze + attr_reader :redirected_path + + # rubocop:disable Cop/ModuleWithInstanceVariables def wiki? set_project unless defined?(@wiki) @wiki end + # rubocop:disable Cop/ModuleWithInstanceVariables def project set_project unless defined?(@project) @project end - def redirected_path - @redirected_path - end - def ssh_authentication_abilities [ :read_project, @@ -57,6 +57,7 @@ module API private + # rubocop:disable Cop/ModuleWithInstanceVariables def set_project if params[:gl_repository] @project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository]) diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index 721ed97bb6b..9e01eed06f3 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -37,6 +37,7 @@ module ExtractsPath # # Returns an Array where the first value is the tree-ish and the second is the # path + # rubocop:disable Cop/ModuleWithInstanceVariables def extract_ref(id) pair = ['', ''] @@ -104,6 +105,7 @@ module ExtractsPath # # Automatically renders `not_found!` if a valid tree path could not be # resolved (e.g., when a user inserts an invalid path or ref). + # rubocop:disable Cop/ModuleWithInstanceVariables def assign_ref_vars # assign allowed options allowed_options = ["filter_ref"] @@ -132,6 +134,7 @@ module ExtractsPath render_404 end + # rubocop:disable Cop/ModuleWithInstanceVariables def tree @tree ||= @repo.tree(@commit.id, @path) end @@ -145,6 +148,7 @@ module ExtractsPath id end + # rubocop:disable Cop/ModuleWithInstanceVariables def ref_names return [] unless @project diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb index 754a45c3257..65e3e672894 100644 --- a/lib/gitlab/cache/request_cache.rb +++ b/lib/gitlab/cache/request_cache.rb @@ -45,6 +45,7 @@ module Gitlab klass.prepend(extension) end + # rubocop:disable Cop/ModuleWithInstanceVariables def request_cache_key(&block) if block_given? @request_cache_key = block diff --git a/lib/gitlab/ci/charts.rb b/lib/gitlab/ci/charts.rb index 7df7b542d91..fe2fd08a67a 100644 --- a/lib/gitlab/ci/charts.rb +++ b/lib/gitlab/ci/charts.rb @@ -2,6 +2,7 @@ module Gitlab module Ci module Charts module DailyInterval + # rubocop:disable Cop/ModuleWithInstanceVariables def grouped_count(query) query .group("DATE(#{::Ci::Pipeline.table_name}.created_at)") diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index 68b6742385a..2c96e5f65d7 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -24,6 +24,7 @@ module Gitlab end end + # rubocop:disable Cop/ModuleWithInstanceVariables def compose!(deps = nil) return unless valid? diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index 5ced778d311..1850c652c09 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -12,6 +12,7 @@ module Gitlab end end + # rubocop:disable Cop/ModuleWithInstanceVariables def errors @validator.messages + descendants.flat_map(&:errors) end diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 642f0944354..4e0dadcc3c7 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -52,6 +52,7 @@ module Gitlab ::ApplicationSetting.create_from_defaults || in_memory_application_settings end + # rubocop:disable Cop/ModuleWithInstanceVariables def in_memory_application_settings @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError diff --git a/lib/gitlab/cycle_analytics/base_event_fetcher.rb b/lib/gitlab/cycle_analytics/base_event_fetcher.rb index ab115afcaa5..bec201f309c 100644 --- a/lib/gitlab/cycle_analytics/base_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/base_event_fetcher.rb @@ -59,6 +59,12 @@ module Gitlab nil end + def load_allowed_ids + allowed_ids_finder_class + .new(@options[:current_user], project_id: @project.id) + .execute.where(id: event_result_ids).pluck(:id) + end + def event_result_ids event_result.map { |event| event['id'] } end diff --git a/lib/gitlab/cycle_analytics/base_query.rb b/lib/gitlab/cycle_analytics/base_query.rb index 58729d3ced8..52fdae84c58 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -11,6 +11,7 @@ module Gitlab @base_query ||= stage_query end + # rubocop:disable Cop/ModuleWithInstanceVariables def stage_query query = mr_closing_issues_table.join(issue_table).on(issue_table[:id].eq(mr_closing_issues_table[:issue_id])) .join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id])) diff --git a/lib/gitlab/cycle_analytics/code_event_fetcher.rb b/lib/gitlab/cycle_analytics/code_event_fetcher.rb index d5bf6149749..39df80352b6 100644 --- a/lib/gitlab/cycle_analytics/code_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/code_event_fetcher.rb @@ -1,8 +1,6 @@ module Gitlab module CycleAnalytics class CodeEventFetcher < BaseEventFetcher - include MergeRequestAllowed - def initialize(*args) @projections = [mr_table[:title], mr_table[:iid], @@ -20,6 +18,14 @@ module Gitlab def serialize(event) AnalyticsMergeRequestSerializer.new(project: @project).represent(event) end + + def allowed_ids + load_allowed_ids + end + + def allowed_ids_finder_class + MergeRequestsFinder + end end end end diff --git a/lib/gitlab/cycle_analytics/issue_allowed.rb b/lib/gitlab/cycle_analytics/issue_allowed.rb deleted file mode 100644 index a7652a70641..00000000000 --- a/lib/gitlab/cycle_analytics/issue_allowed.rb +++ /dev/null @@ -1,9 +0,0 @@ -module Gitlab - module CycleAnalytics - module IssueAllowed - def allowed_ids - @allowed_ids ||= IssuesFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id) - end - end - end -end diff --git a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb index 3df9cbdcfce..cc79e2dfe88 100644 --- a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb @@ -1,8 +1,6 @@ module Gitlab module CycleAnalytics class IssueEventFetcher < BaseEventFetcher - include IssueAllowed - def initialize(*args) @projections = [issue_table[:title], issue_table[:iid], @@ -18,6 +16,14 @@ module Gitlab def serialize(event) AnalyticsIssueSerializer.new(project: @project).represent(event) end + + def allowed_ids + load_allowed_ids + end + + def allowed_ids_finder_class + IssuesFinder + end end end end diff --git a/lib/gitlab/cycle_analytics/merge_request_allowed.rb b/lib/gitlab/cycle_analytics/merge_request_allowed.rb deleted file mode 100644 index 28f6db44759..00000000000 --- a/lib/gitlab/cycle_analytics/merge_request_allowed.rb +++ /dev/null @@ -1,9 +0,0 @@ -module Gitlab - module CycleAnalytics - module MergeRequestAllowed - def allowed_ids - @allowed_ids ||= MergeRequestsFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id) - end - end - end -end diff --git a/lib/gitlab/cycle_analytics/production_helper.rb b/lib/gitlab/cycle_analytics/production_helper.rb index d693443bfa4..9a05c910ba0 100644 --- a/lib/gitlab/cycle_analytics/production_helper.rb +++ b/lib/gitlab/cycle_analytics/production_helper.rb @@ -1,6 +1,7 @@ module Gitlab module CycleAnalytics module ProductionHelper + # rubocop:disable Cop/ModuleWithInstanceVariables def stage_query super.where(mr_metrics_table[:first_deployed_to_production_at].gteq(@options[:from])) end diff --git a/lib/gitlab/cycle_analytics/review_event_fetcher.rb b/lib/gitlab/cycle_analytics/review_event_fetcher.rb index 4c7b3f4467f..5a7f1eb00b3 100644 --- a/lib/gitlab/cycle_analytics/review_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/review_event_fetcher.rb @@ -1,8 +1,6 @@ module Gitlab module CycleAnalytics class ReviewEventFetcher < BaseEventFetcher - include MergeRequestAllowed - def initialize(*args) @projections = [mr_table[:title], mr_table[:iid], @@ -14,9 +12,19 @@ module Gitlab super(*args) end + private + def serialize(event) AnalyticsMergeRequestSerializer.new(project: @project).represent(event) end + + def allowed_ids + load_allowed_ids + end + + def allowed_ids_finder_class + MergeRequestsFinder + end end end end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb index 5481024db8e..6959fa74293 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb @@ -3,6 +3,7 @@ module Gitlab module RenameReservedPathsMigration module V1 module MigrationClasses + # rubocop:disable Cop/ModuleWithInstanceVariables module Routable def full_path if route && route.path.present? diff --git a/lib/gitlab/emoji.rb b/lib/gitlab/emoji.rb index e3e36b35ce9..89cf659bce4 100644 --- a/lib/gitlab/emoji.rb +++ b/lib/gitlab/emoji.rb @@ -31,8 +31,7 @@ module Gitlab end def emoji_unicode_version(name) - @emoji_unicode_versions_by_name ||= JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json'))) - @emoji_unicode_versions_by_name[name] + emoji_unicode_versions_by_name[name] end def normalize_emoji_name(name) @@ -56,5 +55,12 @@ module Gitlab ActionController::Base.helpers.content_tag('gl-emoji', emoji_info['moji'], title: emoji_info['description'], data: data) end + + private + + def emoji_unicode_versions_by_name + @emoji_unicode_versions_by_name ||= + JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json'))) + end end end diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb index d41fe78daa1..1ccca13ce2f 100644 --- a/lib/gitlab/git/popen.rb +++ b/lib/gitlab/git/popen.rb @@ -16,8 +16,8 @@ module Gitlab vars['PWD'] = path options = { chdir: path } - @cmd_output = "" - @cmd_status = 0 + cmd_output = "" + cmd_status = 0 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| yield(stdin) if block_given? stdin.close @@ -25,14 +25,14 @@ module Gitlab if lazy_block return lazy_block.call(stdout.lazy) else - @cmd_output << stdout.read + cmd_output << stdout.read end - @cmd_output << stderr.read - @cmd_status = wait_thr.value.exitstatus + cmd_output << stderr.read + cmd_status = wait_thr.value.exitstatus end - [@cmd_output, @cmd_status] + [cmd_output, cmd_status] end def popen_with_timeout(cmd, timeout, path, vars = {}) diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 90942774a2e..5ac57c5775a 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -30,6 +30,7 @@ module Gitlab execute(%W(tar -#{options} #{archive} -C #{dir})) end + # rubocop:disable Cop/ModuleWithInstanceVariables def execute(cmd) output, status = Gitlab::Popen.popen(cmd) @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index 7b06bb953aa..c4dc061eda1 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -149,6 +149,7 @@ module Gitlab # When enabled this should be set before being used as the usual pattern # "@foo ||= bar" is _not_ thread-safe. + # rubocop:disable Cop/ModuleWithInstanceVariables def pool if influx_metrics_enabled? if @pool.nil? diff --git a/lib/gitlab/metrics/prometheus.rb b/lib/gitlab/metrics/prometheus.rb index 460dab47276..b5f9dafccab 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/lib/gitlab/metrics/prometheus.rb @@ -13,6 +13,7 @@ module Gitlab ::File.writable?(multiprocess_files_dir) end + # rubocop:disable Cop/ModuleWithInstanceVariables def prometheus_metrics_enabled? return @prometheus_metrics_enabled if defined?(@prometheus_metrics_enabled) diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 22f8dd669d0..cd8b2eba6c4 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module PathRegex extend self diff --git a/lib/gitlab/slash_commands/presenters/issue_base.rb b/lib/gitlab/slash_commands/presenters/issue_base.rb index 341f2aabdd0..5aaf3655396 100644 --- a/lib/gitlab/slash_commands/presenters/issue_base.rb +++ b/lib/gitlab/slash_commands/presenters/issue_base.rb @@ -10,14 +10,17 @@ module Gitlab issuable.open? ? 'Open' : 'Closed' end + # rubocop:disable Cop/ModuleWithInstanceVariables def project @resource.project end + # rubocop:disable Cop/ModuleWithInstanceVariables def author @resource.author end + # rubocop:disable Cop/ModuleWithInstanceVariables def fields [ { diff --git a/lib/tasks/gitlab/task_helpers.rb b/lib/tasks/gitlab/task_helpers.rb index 8a63f486fa3..2feacbb4a09 100644 --- a/lib/tasks/gitlab/task_helpers.rb +++ b/lib/tasks/gitlab/task_helpers.rb @@ -104,6 +104,7 @@ module Gitlab Gitlab.config.gitlab.user end + # rubocop:disable Cop/ModuleWithInstanceVariables def gitlab_user? return @is_gitlab_user unless @is_gitlab_user.nil? @@ -111,6 +112,7 @@ module Gitlab @is_gitlab_user = current_user == gitlab_user end + # rubocop:disable Cop/ModuleWithInstanceVariables def warn_user_is_not_gitlab return if @warned_user_not_gitlab diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb new file mode 100644 index 00000000000..95e612b58e8 --- /dev/null +++ b/rubocop/cop/module_with_instance_variables.rb @@ -0,0 +1,73 @@ +module RuboCop + module Cop + class ModuleWithInstanceVariables < RuboCop::Cop::Cop + MSG = <<~EOL.freeze + Do not use instance variables in a module. Please read this + for the rationale behind it: + + doc/development/module_with_instance_variables.md + + If you think the use for this is fine, please just add: + # rubocop:disable Cop/ModuleWithInstanceVariables + EOL + + def on_module(node) + return if + rails_helper?(node) || rails_mailer?(node) || spec_helper?(node) + + check_method_definition(node) + + # Not sure why some module would have an extra begin wrapping around + node.each_child_node(:begin) do |begin_node| + check_method_definition(begin_node) + end + end + + private + + # We ignore Rails helpers right now because it's hard to workaround it + def rails_helper?(node) + node.source_range.source_buffer.name =~ + %r{app/helpers/\w+_helper.rb\z} + end + + # We ignore Rails mailers right now because it's hard to workaround it + def rails_mailer?(node) + node.source_range.source_buffer.name =~ + %r{app/mailers/emails/} + end + + # We ignore spec helpers because it usually doesn't matter + def spec_helper?(node) + node.source_range.source_buffer.name =~ + %r{spec/support/|features/steps/} + end + + def check_method_definition(node) + node.each_child_node(:def) do |definition| + # We allow this pattern: + # def f + # @f ||= true + # end + if only_ivar_or_assignment?(definition) + # We don't allow if any other ivar is used + definition.each_descendant(:ivar) do |offense| + add_offense(offense, :expression) + end + else + definition.each_descendant(:ivar, :ivasgn) do |offense| + add_offense(offense, :expression) + end + end + end + end + + def only_ivar_or_assignment?(definition) + node = definition.child_nodes.last + + definition.child_nodes.size == 2 && + node.or_asgn_type? && node.child_nodes.first.ivasgn_type? + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 4ebbe010e90..7db56349cd7 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -5,6 +5,9 @@ require_relative 'cop/gem_fetcher' require_relative 'cop/in_batches' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' +require_relative 'cop/active_record_dependent' +require_relative 'cop/in_batches' +require_relative 'cop/module_with_instance_variables' require_relative 'cop/redirect_with_status' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column_with_default_to_large_table' diff --git a/spec/rubocop/cop/module_with_instance_variables_spec.rb b/spec/rubocop/cop/module_with_instance_variables_spec.rb new file mode 100644 index 00000000000..bac39117dba --- /dev/null +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -0,0 +1,172 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/module_with_instance_variables' + +describe RuboCop::Cop::ModuleWithInstanceVariables do + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples('registering offense') do + it 'registers an offense when instance variable is used in a module' do + inspect_source(cop, source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + shared_examples('not registering offense') do + it 'does not register offenses' do + inspect_source(cop, source) + + expect(cop.offenses).to be_empty + end + end + + context 'when source is a regular module' do + let(:source) do + <<~RUBY + module M + def f + @f = true + end + end + RUBY + 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 + end + end + end + RUBY + 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 + end + end + end + RUBY + end + + let(:offending_lines) { [4, 12] } + + it_behaves_like 'registering offense' + end + + context 'with regular ivar assignment' do + let(:source) do + <<~RUBY + module M + def f + @f = true + end + end + RUBY + end + + context 'when source is offending but it is a rails helper' do + before do + allow(cop).to receive(:rails_helper?).and_return(true) + end + + it_behaves_like 'not registering offense' + end + + context 'when source is offending but it is a rails mailer' do + before do + allow(cop).to receive(:rails_mailer?).and_return(true) + end + + it_behaves_like 'not registering offense' + end + + context 'when source is offending but it is a spec helper' do + before do + allow(cop).to receive(:spec_helper?).and_return(true) + end + + it_behaves_like 'not registering offense' + end + end + + context 'when source is using simple or ivar assignment' do + let(:source) do + <<~RUBY + module M + def f + @f ||= true + end + end + RUBY + 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) + end + end + RUBY + 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 + end + end + RUBY + end + + let(:offending_lines) { [3, 4] } + + it_behaves_like 'registering offense' + end +end |