diff options
81 files changed, 475 insertions, 34 deletions
diff --git a/app/controllers/concerns/boards_responses.rb b/app/controllers/concerns/boards_responses.rb index 2c9c095a5d7..05f8a6aed69 100644 --- a/app/controllers/concerns/boards_responses.rb +++ b/app/controllers/concerns/boards_responses.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module BoardsResponses def authorize_read_list authorize_action_for!(board.parent, :read_list) diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 782f0be9c4a..2b7f3ba0feb 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module CreatesCommit extend ActiveSupport::Concern diff --git a/app/controllers/concerns/cycle_analytics_params.rb b/app/controllers/concerns/cycle_analytics_params.rb index 1ab107168c0..0d572aab901 100644 --- a/app/controllers/concerns/cycle_analytics_params.rb +++ b/app/controllers/concerns/cycle_analytics_params.rb @@ -1,6 +1,7 @@ module CycleAnalyticsParams extend ActiveSupport::Concern + # rubocop:disable Cop/ModuleWithInstanceVariables def options(params) @options ||= { from: start_date(params), current_user: current_user } end diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 4079072a930..1539f2dcbdc 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module IssuableActions extend ActiveSupport::Concern diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 0d0e53d4b76..edce4b9481c 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module IssuableCollections extend ActiveSupport::Concern include SortingHelper diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb index 404559c8707..fb7e110cf99 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/app/controllers/concerns/issues_action.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module IssuesAction extend ActiveSupport::Concern include IssuableCollections diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 2b6afaa6233..608a580e1ac 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -90,6 +90,7 @@ module LfsRequest has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project) end + # rubocop:disable Cop/ModuleWithInstanceVariables def storage_project @storage_project ||= begin result = project @@ -103,6 +104,7 @@ module LfsRequest end end + # rubocop:disable Cop/ModuleWithInstanceVariables def objects @objects ||= (params[:objects] || []).to_a end diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index c6b1e443de6..105e9274f7e 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -76,6 +76,7 @@ module MembershipActions end end + # rubocop:disable Cop/ModuleWithInstanceVariables def source_type @source_type ||= membershipable.class.to_s.humanize(capitalize: false) end diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb index d3c8e4888bc..d8e9e1a4479 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/app/controllers/concerns/merge_requests_action.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module MergeRequestsAction extend ActiveSupport::Concern include IssuableCollections 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 18fd8eb114d..f113cb3d381 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..b209d1be87c 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -13,6 +13,7 @@ module OauthApplications end end + # rubocop:disable Cop/ModuleWithInstanceVariables def load_scopes @scopes = Doorkeeper.configuration.scopes 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/requires_whitelisted_monitoring_client.rb b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb index 0218ac83441..bf681f6dba4 100644 --- a/app/controllers/concerns/requires_whitelisted_monitoring_client.rb +++ b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb @@ -17,6 +17,7 @@ module RequiresWhitelistedMonitoringClient ip_whitelist.any? { |e| e.include?(Gitlab::RequestContext.client_ip) } end + # rubocop:disable Cop/ModuleWithInstanceVariables def ip_whitelist @ip_whitelist ||= Settings.monitoring.ip_whitelist.map(&IPAddr.method(:new)) end 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/ignorable_column.rb b/app/models/concerns/ignorable_column.rb index eb9f3423e48..3a3afbcd366 100644 --- a/app/models/concerns/ignorable_column.rb +++ b/app/models/concerns/ignorable_column.rb @@ -17,6 +17,7 @@ module IgnorableColumn super.reject { |column| ignored_columns.include?(column.name) } end + # rubocop:disable Cop/ModuleWithInstanceVariables def ignored_columns @ignored_columns ||= Set.new end diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 1db6b2d2fa2..b2dca51f5de 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -5,6 +5,7 @@ # # Used by Issue, Note, MergeRequest, and Commit. # +# # rubocop:disable Cop/ModuleWithInstanceVariables module Mentionable extend ActiveSupport::Concern 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 1c4ddabcad5..143f4a12bba 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -12,6 +12,7 @@ module Noteable # # noteable.class # => MergeRequest # noteable.human_class_name # => "merge request" + # rubocop:disable Cop/ModuleWithInstanceVariables def human_class_name @human_class_name ||= base_class_name.titleize.downcase end @@ -34,6 +35,7 @@ module Noteable delegate :find_discussion, to: :discussion_notes + # rubocop:disable Cop/ModuleWithInstanceVariables def discussions @discussions ||= discussion_notes .inc_relations_for_view @@ -46,6 +48,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) @@ -67,6 +70,7 @@ module Noteable discussions_resolvable? && !discussions_resolved? end + # rubocop:disable Cop/ModuleWithInstanceVariables def discussions_to_be_resolved @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?) end diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index ce69fd34ac5..e971b2aafdd 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -55,6 +55,7 @@ module Participable # This method processes attributes of objects in breadth-first order. # # Returns an Array of User instances. + # rubocop:disable Cop/ModuleWithInstanceVariables def participants(current_user = nil) @participants ||= Hash.new do |hash, user| hash[user] = raw_participants(user) diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 454374121f3..cd3889c1385 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -55,6 +55,7 @@ module ProtectedRef private + # rubocop:disable Cop/ModuleWithInstanceVariables def ref_matcher @ref_matcher ||= ProtectedRefMatcher.new(self) end diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index e961c97e337..951dd925292 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module RelativePositioning extend ActiveSupport::Concern diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index f006a271327..09bb2823ab9 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module ResolvableDiscussion extend ActiveSupport::Concern diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index f5048d17d80..a68f9188e80 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -1,5 +1,6 @@ # Store object full path in separate table for easy lookup and uniq validation # Object must have name and path db fields and respond to parent and parent_changed? methods. +# rubocop:disable Cop/ModuleWithInstanceVariables module Routable extend ActiveSupport::Concern 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/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 5ab5c80a2f5..3823653b386 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -49,6 +49,7 @@ module Storage private + # rubocop:disable Cop/ModuleWithInstanceVariables def old_repository_storage_paths @old_repository_storage_paths ||= repository_storage_paths end diff --git a/app/models/concerns/strip_attribute.rb b/app/models/concerns/strip_attribute.rb index 8806ebe897a..5189f736991 100644 --- a/app/models/concerns/strip_attribute.rb +++ b/app/models/concerns/strip_attribute.rb @@ -17,6 +17,7 @@ module StripAttribute strip_attrs.concat(attrs) end + # rubocop:disable Cop/ModuleWithInstanceVariables def strip_attrs @strip_attrs ||= [] end diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index 25e2d8ea24e..298f7b61047 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -6,6 +6,7 @@ require 'task_list/filter' # bugs". # # Used by MergeRequest and Issue +# rubocop:disable Cop/ModuleWithInstanceVariables module Taskable COMPLETED = 'completed'.freeze INCOMPLETE = 'incomplete'.freeze diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index b517ddaebd7..75f7c81fa4e 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -4,7 +4,7 @@ # # Used by Issue and MergeRequest. # - +# rubocop:disable Cop/ModuleWithInstanceVariables module TimeTrackable extend ActiveSupport::Concern 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..e61e7e20476 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -6,6 +6,7 @@ # Dependencies: # - params with :request # +# rubocop:disable Cop/ModuleWithInstanceVariables module SpamCheckService def filter_spam_check_params @request = params.delete(:request) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1f66a2668f9..47ca8c3a004 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -662,6 +662,7 @@ module SystemNoteService Rack::Utils.escape_html(text) end + # rubocop:disable Cop/ModuleWithInstanceVariables def url_helpers @url_helpers ||= Gitlab::Routing.url_helpers end 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..b839fd177af 100644 --- a/config/initializers/rugged_use_gitlab_git_attributes.rb +++ b/config/initializers/rugged_use_gitlab_git_attributes.rb @@ -11,6 +11,7 @@ # anyway, and there is no great efficiency gain from just fetching the listed # attributes with our implementation, so we ignore the additional arguments. # +# rubocop:disable Cop/ModuleWithInstanceVariables module Rugged class Repository module UseGitlabGitAttributes diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md new file mode 100644 index 00000000000..d38f8c4d137 --- /dev/null +++ b/doc/development/module_with_instance_variables.md @@ -0,0 +1,183 @@ +## Usually modules with instance variables 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 +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. + +### Exceptions + +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. Unfortunately it's a bit hard to code this principle in the cop, so +for now we rely on people turning off the cops, if they think that the use +conform this rule. + +Here's an acceptable case: + +``` ruby +# This is ok, as long as `@attributes` is never used anywhere else. +# Consider adding some prefix or suffix to avoid name conflicts though. +# rubocop:disable Cop/ModuleWithInstanceVariables +module Rugged + class Repository + module UseGitlabGitAttributes + def fetch_attributes(name, *) + @attributes ||= Gitlab::Git::Attributes.new(path) + @attributes.attributes(name) + end + end + + prepend UseGitlabGitAttributes + end +end +``` + +Here's a bad example which we should 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 reduce 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 608d988755c..d99078d4fa3 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/after_commit_queue.rb b/lib/after_commit_queue.rb index 4750a2c373a..f3fba4fe389 100644 --- a/lib/after_commit_queue.rb +++ b/lib/after_commit_queue.rb @@ -20,6 +20,7 @@ module AfterCommitQueue end end + # rubocop:disable Cop/ModuleWithInstanceVariables def _after_commit_queue @after_commit_queue ||= [] end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index c4c0fdda665..05f55097a80 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -2,6 +2,7 @@ require 'rack/oauth2' +# rubocop:disable Cop/ModuleWithInstanceVariables module API module APIGuard extend ActiveSupport::Concern diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 00dbc2aee7a..49e659d3d27 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module API module Helpers include Gitlab::Utils diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 4c0db4d42b1..a187517a66a 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module API module Helpers module InternalHelpers @@ -57,6 +58,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/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 282af32ca94..1b21594487d 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -21,6 +21,7 @@ module API forbidden! unless current_runner end + # rubocop:disable Cop/ModuleWithInstanceVariables def current_runner @runner ||= ::Ci::Runner.find_by_token(params[:token].to_s) end diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index 721ed97bb6b..f3e5b1c1109 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -1,5 +1,6 @@ # Module providing methods for dealing with separating a tree-ish string and a # file path string when combined in a request parameter +# rubocop:disable Cop/ModuleWithInstanceVariables module ExtractsPath # Raised when given an invalid file path InvalidPathError = Class.new(StandardError) 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..a7040a8fa03 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)") @@ -9,6 +10,7 @@ module Gitlab .transform_keys { |date| date.strftime(@format) } end + # rubocop:disable Cop/ModuleWithInstanceVariables def interval_step @interval_step ||= 1.day end @@ -28,6 +30,7 @@ module Gitlab end end + # rubocop:disable Cop/ModuleWithInstanceVariables def interval_step @interval_step ||= 1.month end diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index 68b6742385a..e34f4c9e101 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -13,6 +13,7 @@ module Gitlab # script: ... # artifacts: ... # + # rubocop:disable Cop/ModuleWithInstanceVariables module Configurable extend ActiveSupport::Concern diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index 5ced778d311..524d349c094 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Ci class Config diff --git a/lib/gitlab/ci/model.rb b/lib/gitlab/ci/model.rb index 3994a50772b..213301d245e 100644 --- a/lib/gitlab/ci/model.rb +++ b/lib/gitlab/ci/model.rb @@ -5,6 +5,7 @@ module Gitlab "ci_" end + # rubocop:disable Cop/ModuleWithInstanceVariables def model_name @model_name ||= ActiveModel::Name.new(self, nil, self.name.split("::").last) 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..3f6fb227a67 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -7,6 +7,7 @@ module Gitlab private + # rubocop:disable Cop/ModuleWithInstanceVariables def base_query @base_query ||= stage_query end 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..cd7ee39d9ca 100644 --- a/lib/gitlab/cycle_analytics/production_helper.rb +++ b/lib/gitlab/cycle_analytics/production_helper.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module CycleAnalytics module ProductionHelper 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/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index 32c5caf93e8..f077d64d75e 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -12,6 +12,7 @@ module Gitlab raise NotImplementedError end + # rubocop:disable Cop/ModuleWithInstanceVariables def message @message ||= process_message end diff --git a/lib/gitlab/emoji.rb b/lib/gitlab/emoji.rb index e3e36b35ce9..c8689d85c0c 100644 --- a/lib/gitlab/emoji.rb +++ b/lib/gitlab/emoji.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Emoji extend self diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb index 25fa62ce4bd..10c15b316f5 100644 --- a/lib/gitlab/git/popen.rb +++ b/lib/gitlab/git/popen.rb @@ -13,15 +13,15 @@ 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| - @cmd_output << stdout.read - @cmd_output << stderr.read - @cmd_status = wait_thr.value.exitstatus + cmd_output << stdout.read + cmd_output << stderr.read + cmd_status = wait_thr.value.exitstatus end - [@cmd_output, @cmd_status] + [cmd_output, cmd_status] end end end diff --git a/lib/gitlab/identifier.rb b/lib/gitlab/identifier.rb index 94678b6ec40..54fd9d15e7b 100644 --- a/lib/gitlab/identifier.rb +++ b/lib/gitlab/identifier.rb @@ -52,6 +52,7 @@ module Gitlab end end + # rubocop:disable Cop/ModuleWithInstanceVariables def identification_cache @identification_cache ||= { email: {}, 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..4d0f79ef163 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Metrics module InfluxDb diff --git a/lib/gitlab/metrics/prometheus.rb b/lib/gitlab/metrics/prometheus.rb index 460dab47276..476aad2f4dd 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/lib/gitlab/metrics/prometheus.rb @@ -1,5 +1,6 @@ require 'prometheus/client' +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Metrics module Prometheus diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 7c02c9c5c48..6df9d60721e 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/prometheus/additional_metrics_parser.rb b/lib/gitlab/prometheus/additional_metrics_parser.rb index cb95daf2260..37112ca3cdb 100644 --- a/lib/gitlab/prometheus/additional_metrics_parser.rb +++ b/lib/gitlab/prometheus/additional_metrics_parser.rb @@ -26,6 +26,7 @@ module Gitlab load_yaml_file&.map(&:deep_symbolize_keys).freeze end + # rubocop:disable Cop/ModuleWithInstanceVariables def load_yaml_file @loaded_yaml_file ||= YAML.load_file(Rails.root.join('config/prometheus/additional_metrics.yml')) end diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index 7ac6162b54d..6e377a24e57 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -56,6 +56,7 @@ module Gitlab query end + # rubocop:disable Cop/ModuleWithInstanceVariables def available_metrics @available_metrics ||= client_label_values || [] end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 58f6245579a..fdd5c86c698 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Regex extend self diff --git a/lib/gitlab/slash_commands/presenters/issue_base.rb b/lib/gitlab/slash_commands/presenters/issue_base.rb index 341f2aabdd0..2cec307867c 100644 --- a/lib/gitlab/slash_commands/presenters/issue_base.rb +++ b/lib/gitlab/slash_commands/presenters/issue_base.rb @@ -1,3 +1,4 @@ +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module SlashCommands module Presenters diff --git a/lib/gitlab/themes.rb b/lib/gitlab/themes.rb index d43eff5ba4a..7805f89831b 100644 --- a/lib/gitlab/themes.rb +++ b/lib/gitlab/themes.rb @@ -72,6 +72,7 @@ module Gitlab private + # rubocop:disable Cop/ModuleWithInstanceVariables def default_id @default_id ||= begin id = Gitlab.config.gitlab.default_theme.to_i diff --git a/lib/tasks/gitlab/task_helpers.rb b/lib/tasks/gitlab/task_helpers.rb index 8a63f486fa3..24b65630b87 100644 --- a/lib/tasks/gitlab/task_helpers.rb +++ b/lib/tasks/gitlab/task_helpers.rb @@ -1,5 +1,6 @@ require 'rainbow/ext/string' +# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab TaskFailedError = Class.new(StandardError) TaskAbortedByUserError = Class.new(StandardError) diff --git a/qa/qa/runtime/namespace.rb b/qa/qa/runtime/namespace.rb index e4910b63a14..a2480569a1e 100644 --- a/qa/qa/runtime/namespace.rb +++ b/qa/qa/runtime/namespace.rb @@ -3,6 +3,7 @@ module QA module Namespace extend self + # rubocop:disable Cop/ModuleWithInstanceVariables def time @time ||= Time.now end diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb new file mode 100644 index 00000000000..6ed1b986fdd --- /dev/null +++ b/rubocop/cop/module_with_instance_variables.rb @@ -0,0 +1,55 @@ +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| + definition.each_descendant(:ivar, :ivasgn) do |offense| + add_offense(offense, :expression) + end + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 1b6e8991a17..bb0d25f3c48 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -6,6 +6,7 @@ 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/migration/add_column' require_relative 'cop/migration/add_column_with_default_to_large_table' require_relative 'cop/migration/add_concurrent_foreign_key' 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..ce2e156e423 --- /dev/null +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -0,0 +1,117 @@ +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 + + 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 'when source is offending but it is a rails helper' do + before do + allow(cop).to receive(:rails_helper?).and_return(true) + end + + it 'does not register offenses' do + inspect_source(cop, <<~RUBY) + module M + def f + @f ||= true + end + end + RUBY + + expect(cop.offenses).to be_empty + end + 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 'does not register offenses' do + inspect_source(cop, <<~RUBY) + module M + def f + @f = true + end + end + RUBY + + expect(cop.offenses).to be_empty + end + end +end |