From 9ae92b8caa6c11d8860f86b7d6378062215d1b72 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 12 Jul 2017 02:29:33 +0800 Subject: [PATCH 01/16] Add cop to make sure we don't use ivar in a module --- app/controllers/concerns/boards_responses.rb | 1 + app/controllers/concerns/creates_commit.rb | 1 + .../concerns/cycle_analytics_params.rb | 1 + app/controllers/concerns/issuable_actions.rb | 1 + .../concerns/issuable_collections.rb | 1 + app/controllers/concerns/issues_action.rb | 1 + app/controllers/concerns/lfs_request.rb | 2 + .../concerns/membership_actions.rb | 1 + .../concerns/merge_requests_action.rb | 1 + app/controllers/concerns/milestone_actions.rb | 1 + app/controllers/concerns/notes_actions.rb | 1 + .../concerns/oauth_applications.rb | 1 + app/controllers/concerns/renders_commits.rb | 1 + app/controllers/concerns/renders_notes.rb | 1 + .../requires_whitelisted_monitoring_client.rb | 1 + app/controllers/concerns/service_params.rb | 1 + app/controllers/concerns/snippets_actions.rb | 1 + app/controllers/concerns/spammable_actions.rb | 1 + .../concerns/toggle_subscription_action.rb | 1 + app/models/concerns/ignorable_column.rb | 1 + app/models/concerns/mentionable.rb | 1 + app/models/concerns/milestoneish.rb | 1 + app/models/concerns/noteable.rb | 4 + app/models/concerns/participable.rb | 1 + app/models/concerns/protected_ref.rb | 1 + app/models/concerns/relative_positioning.rb | 1 + app/models/concerns/resolvable_discussion.rb | 1 + app/models/concerns/routable.rb | 1 + app/models/concerns/spammable.rb | 2 +- .../concerns/storage/legacy_namespace.rb | 1 + app/models/concerns/strip_attribute.rb | 1 + app/models/concerns/taskable.rb | 1 + app/models/concerns/time_trackable.rb | 2 +- .../concerns/issues/resolve_discussions.rb | 3 + app/services/spam_check_service.rb | 1 + app/services/system_note_service.rb | 1 + app/workers/concerns/new_issuable.rb | 2 + .../fix_local_cache_middleware.rb | 1 + config/initializers/rspec_profiling.rb | 1 + .../rugged_use_gitlab_git_attributes.rb | 1 + .../module_with_instance_variables.md | 183 ++++++++++++++++++ features/support/env.rb | 4 +- lib/after_commit_queue.rb | 1 + lib/api/api_guard.rb | 1 + lib/api/helpers.rb | 1 + lib/api/helpers/internal_helpers.rb | 2 + lib/api/helpers/runner.rb | 1 + lib/extracts_path.rb | 1 + lib/gitlab/cache/request_cache.rb | 1 + lib/gitlab/ci/charts.rb | 3 + lib/gitlab/ci/config/entry/configurable.rb | 1 + lib/gitlab/ci/config/entry/validatable.rb | 1 + lib/gitlab/ci/model.rb | 1 + lib/gitlab/current_settings.rb | 1 + .../cycle_analytics/base_event_fetcher.rb | 6 + lib/gitlab/cycle_analytics/base_query.rb | 1 + .../cycle_analytics/code_event_fetcher.rb | 10 +- lib/gitlab/cycle_analytics/issue_allowed.rb | 9 - .../cycle_analytics/issue_event_fetcher.rb | 10 +- .../cycle_analytics/merge_request_allowed.rb | 9 - .../cycle_analytics/production_helper.rb | 1 + .../cycle_analytics/review_event_fetcher.rb | 12 +- .../v1/migration_classes.rb | 1 + lib/gitlab/email/handler/reply_processing.rb | 1 + lib/gitlab/emoji.rb | 1 + lib/gitlab/git/popen.rb | 12 +- lib/gitlab/identifier.rb | 1 + lib/gitlab/import_export/command_line_util.rb | 1 + lib/gitlab/metrics/influx_db.rb | 1 + lib/gitlab/metrics/prometheus.rb | 1 + lib/gitlab/path_regex.rb | 1 + .../prometheus/additional_metrics_parser.rb | 1 + .../queries/query_additional_metrics.rb | 1 + lib/gitlab/regex.rb | 1 + .../slash_commands/presenters/issue_base.rb | 1 + lib/gitlab/themes.rb | 1 + lib/tasks/gitlab/task_helpers.rb | 1 + qa/qa/runtime/namespace.rb | 1 + rubocop/cop/module_with_instance_variables.rb | 55 ++++++ rubocop/rubocop.rb | 1 + .../module_with_instance_variables_spec.rb | 117 +++++++++++ 81 files changed, 475 insertions(+), 34 deletions(-) create mode 100644 doc/development/module_with_instance_variables.md delete mode 100644 lib/gitlab/cycle_analytics/issue_allowed.rb delete mode 100644 lib/gitlab/cycle_analytics/merge_request_allowed.rb create mode 100644 rubocop/cop/module_with_instance_variables.rb create mode 100644 spec/rubocop/cop/module_with_instance_variables_spec.rb 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 From 6a4ee9aa7140862075cafae1ddebd133eec52b5b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Sep 2017 01:25:23 +0800 Subject: [PATCH 02/16] Allow simple ivar ||= form. Update accordingly --- app/controllers/concerns/boards_responses.rb | 3 +- app/controllers/concerns/creates_commit.rb | 6 +- .../concerns/cycle_analytics_params.rb | 1 - app/controllers/concerns/issuable_actions.rb | 5 +- .../concerns/issuable_collections.rb | 3 +- app/controllers/concerns/issues_action.rb | 2 +- app/controllers/concerns/lfs_request.rb | 2 - .../concerns/membership_actions.rb | 1 - .../concerns/merge_requests_action.rb | 2 +- .../concerns/oauth_applications.rb | 3 +- .../requires_whitelisted_monitoring_client.rb | 1 - app/models/concerns/ignorable_column.rb | 1 - app/models/concerns/mentionable.rb | 2 +- app/models/concerns/noteable.rb | 3 - app/models/concerns/participable.rb | 13 +- app/models/concerns/protected_ref.rb | 1 - app/models/concerns/relative_positioning.rb | 5 +- app/models/concerns/resolvable_discussion.rb | 6 +- app/models/concerns/routable.rb | 5 +- .../concerns/storage/legacy_namespace.rb | 1 - app/models/concerns/strip_attribute.rb | 1 - app/models/concerns/taskable.rb | 2 +- app/models/concerns/time_trackable.rb | 4 +- app/services/spam_check_service.rb | 3 +- app/services/system_note_service.rb | 1 - .../rugged_use_gitlab_git_attributes.rb | 2 +- .../module_with_instance_variables.md | 75 ++++++++---- lib/after_commit_queue.rb | 1 - lib/api/api_guard.rb | 8 +- lib/api/helpers.rb | 4 +- lib/api/helpers/internal_helpers.rb | 9 +- lib/api/helpers/runner.rb | 1 - lib/extracts_path.rb | 5 +- lib/gitlab/ci/charts.rb | 2 - lib/gitlab/ci/config/entry/configurable.rb | 2 +- lib/gitlab/ci/config/entry/validatable.rb | 2 +- lib/gitlab/ci/model.rb | 1 - lib/gitlab/cycle_analytics/base_query.rb | 2 +- .../cycle_analytics/production_helper.rb | 2 +- lib/gitlab/email/handler/reply_processing.rb | 1 - lib/gitlab/emoji.rb | 11 +- lib/gitlab/identifier.rb | 1 - lib/gitlab/metrics/influx_db.rb | 2 +- lib/gitlab/metrics/prometheus.rb | 2 +- .../prometheus/additional_metrics_parser.rb | 1 - .../queries/query_additional_metrics.rb | 1 - lib/gitlab/regex.rb | 1 - .../slash_commands/presenters/issue_base.rb | 4 +- lib/gitlab/themes.rb | 1 - lib/tasks/gitlab/task_helpers.rb | 3 +- qa/qa/runtime/namespace.rb | 1 - rubocop/cop/module_with_instance_variables.rb | 22 +++- .../module_with_instance_variables_spec.rb | 111 +++++++++++++----- 53 files changed, 233 insertions(+), 122 deletions(-) diff --git a/app/controllers/concerns/boards_responses.rb b/app/controllers/concerns/boards_responses.rb index 05f8a6aed69..058e4591770 100644 --- a/app/controllers/concerns/boards_responses.rb +++ b/app/controllers/concerns/boards_responses.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module BoardsResponses def authorize_read_list authorize_action_for!(board.parent, :read_list) @@ -24,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 2b7f3ba0feb..0350c9228c9 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -1,7 +1,7 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables 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 @@ -78,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, @@ -94,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) @@ -101,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/cycle_analytics_params.rb b/app/controllers/concerns/cycle_analytics_params.rb index 0d572aab901..1ab107168c0 100644 --- a/app/controllers/concerns/cycle_analytics_params.rb +++ b/app/controllers/concerns/cycle_analytics_params.rb @@ -1,7 +1,6 @@ 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 1539f2dcbdc..0b4b1e65b1d 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module IssuableActions extend ActiveSupport::Concern @@ -8,6 +7,7 @@ module IssuableActions before_action :authorize_admin_issuable!, only: :bulk_update end + # rubocop:disable Cop/ModuleWithInstanceVariables def destroy issuable.destroy destroy_method = "destroy_#{issuable.class.name.underscore}".to_sym @@ -36,6 +36,7 @@ module IssuableActions private + # rubocop:disable Cop/ModuleWithInstanceVariables def render_conflict_response respond_to do |format| format.html do @@ -53,6 +54,7 @@ module IssuableActions end end + # rubocop:disable Cop/ModuleWithInstanceVariables def labels @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute end @@ -63,6 +65,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 edce4b9481c..a95854a1ec1 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module IssuableCollections extend ActiveSupport::Concern include SortingHelper @@ -11,6 +10,7 @@ module IssuableCollections private + # rubocop:disable Cop/ModuleWithInstanceVariables def set_issues_index @collection_type = "Issue" @issues = issues_collection @@ -85,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 fb7e110cf99..a28dd376c80 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/app/controllers/concerns/issues_action.rb @@ -1,8 +1,8 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module IssuesAction extend ActiveSupport::Concern include IssuableCollections + # rubocop:disable Cop/ModuleWithInstanceVariables def issues @label = issues_finder.labels.first diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 608a580e1ac..2b6afaa6233 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -90,7 +90,6 @@ 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 @@ -104,7 +103,6 @@ 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 105e9274f7e..c6b1e443de6 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -76,7 +76,6 @@ 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 d8e9e1a4479..0f61e1ccc06 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/app/controllers/concerns/merge_requests_action.rb @@ -1,8 +1,8 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables 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/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index b209d1be87c..f0a68f23566 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -13,8 +13,7 @@ module OauthApplications end end - # rubocop:disable Cop/ModuleWithInstanceVariables def load_scopes - @scopes = Doorkeeper.configuration.scopes + @scopes ||= Doorkeeper.configuration.scopes end end diff --git a/app/controllers/concerns/requires_whitelisted_monitoring_client.rb b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb index bf681f6dba4..0218ac83441 100644 --- a/app/controllers/concerns/requires_whitelisted_monitoring_client.rb +++ b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb @@ -17,7 +17,6 @@ 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/models/concerns/ignorable_column.rb b/app/models/concerns/ignorable_column.rb index 3a3afbcd366..eb9f3423e48 100644 --- a/app/models/concerns/ignorable_column.rb +++ b/app/models/concerns/ignorable_column.rb @@ -17,7 +17,6 @@ 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 b2dca51f5de..7644f2ea95f 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -5,7 +5,6 @@ # # Used by Issue, Note, MergeRequest, and Commit. # -# # rubocop:disable Cop/ModuleWithInstanceVariables module Mentionable extend ActiveSupport::Concern @@ -44,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/noteable.rb b/app/models/concerns/noteable.rb index 143f4a12bba..9d81a19cbb9 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -12,7 +12,6 @@ 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 @@ -35,7 +34,6 @@ module Noteable delegate :find_discussion, to: :discussion_notes - # rubocop:disable Cop/ModuleWithInstanceVariables def discussions @discussions ||= discussion_notes .inc_relations_for_view @@ -70,7 +68,6 @@ 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 e971b2aafdd..e48bc0be410 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -55,17 +55,18 @@ 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) - 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/protected_ref.rb b/app/models/concerns/protected_ref.rb index cd3889c1385..454374121f3 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -55,7 +55,6 @@ 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 951dd925292..9a7b9cd12b0 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module RelativePositioning extend ActiveSupport::Concern @@ -45,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 @@ -59,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 @@ -74,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 @@ -133,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 09bb2823ab9..56ba4a9a4d0 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module ResolvableDiscussion extend ActiveSupport::Concern @@ -31,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? @@ -47,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? @@ -89,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 a68f9188e80..80a8f63514f 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -1,6 +1,5 @@ # 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 @@ -87,6 +86,7 @@ module Routable end end + # rubocop:disable Cop/ModuleWithInstanceVariables def full_name if route && route.name.present? @full_name ||= route.name @@ -107,6 +107,7 @@ module Routable RequestStore[full_path_key] ||= uncached_full_path end + # rubocop:disable Cop/ModuleWithInstanceVariables def expires_full_path_cache RequestStore.delete(full_path_key) if RequestStore.active? @full_path = nil @@ -122,6 +123,7 @@ module Routable private + # rubocop:disable Cop/ModuleWithInstanceVariables def uncached_full_path if route && route.path.present? @full_path ||= route.path @@ -157,6 +159,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/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 3823653b386..5ab5c80a2f5 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -49,7 +49,6 @@ 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 5189f736991..8806ebe897a 100644 --- a/app/models/concerns/strip_attribute.rb +++ b/app/models/concerns/strip_attribute.rb @@ -17,7 +17,6 @@ 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 298f7b61047..a73de49b9bb 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -6,7 +6,6 @@ require 'task_list/filter' # bugs". # # Used by MergeRequest and Issue -# rubocop:disable Cop/ModuleWithInstanceVariables module Taskable COMPLETED = 'completed'.freeze INCOMPLETE = 'incomplete'.freeze @@ -37,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 75f7c81fa4e..995fa98efac 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -4,7 +4,6 @@ # # Used by Issue and MergeRequest. # -# rubocop:disable Cop/ModuleWithInstanceVariables 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] @@ -50,6 +50,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 @@ -58,6 +59,7 @@ module TimeTrackable timelogs.new(time_spent: time_spent, user: @time_spent_user) end + # rubocop:disable Cop/ModuleWithInstanceVariables def check_negative_time_spent return if time_spent.nil? || time_spent == :reset diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index e61e7e20476..9b2a601f84b 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -6,8 +6,8 @@ # Dependencies: # - params with :request # -# rubocop:disable Cop/ModuleWithInstanceVariables module SpamCheckService + # rubocop:disable Cop/ModuleWithInstanceVariables def filter_spam_check_params @request = params.delete(:request) @api = params.delete(:api) @@ -18,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/services/system_note_service.rb b/app/services/system_note_service.rb index 47ca8c3a004..1f66a2668f9 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -662,7 +662,6 @@ 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/config/initializers/rugged_use_gitlab_git_attributes.rb b/config/initializers/rugged_use_gitlab_git_attributes.rb index b839fd177af..abfa2c354cd 100644 --- a/config/initializers/rugged_use_gitlab_git_attributes.rb +++ b/config/initializers/rugged_use_gitlab_git_attributes.rb @@ -11,10 +11,10 @@ # 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 + # 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 index d38f8c4d137..9fc28d4c343 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -1,4 +1,4 @@ -## Usually modules with instance variables considered harmful +## Modules with instance variables could be considered harmful ### Background @@ -43,7 +43,7 @@ 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 +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 @@ -53,36 +53,67 @@ 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 +### 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. 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. +use. -Here's an acceptable case: +We especially allow the case where a single instance variable is used with +`||=` to setup the value. This would look like: ``` 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 +module M + def f + @f ||= true end end ``` -Here's a bad example which we should rewrite: +Unfortunately it's not easy to code more complex rules into the cop, so +we rely on people's best judge. 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 @@ -146,7 +177,7 @@ 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. +and it also reduces the chance of having name conflicts. ### Things we might need to ignore right now diff --git a/lib/after_commit_queue.rb b/lib/after_commit_queue.rb index f3fba4fe389..4750a2c373a 100644 --- a/lib/after_commit_queue.rb +++ b/lib/after_commit_queue.rb @@ -20,7 +20,6 @@ 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 05f55097a80..9933439c43b 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -2,7 +2,6 @@ require 'rack/oauth2' -# rubocop:disable Cop/ModuleWithInstanceVariables module API module APIGuard extend ActiveSupport::Concern @@ -43,6 +42,8 @@ module API # Helper Methods for Grape Endpoint module HelperMethods + attr_reader :current_user + # Invokes the doorkeeper guard. # # If token is presented and valid, then it sets @current_user. @@ -61,6 +62,7 @@ module API # scopes: (optional) scopes required for this guard. # Defaults to empty array. # + # rubocop:disable Cop/ModuleWithInstanceVariables def doorkeeper_guard(scopes: []) access_token = find_access_token return nil unless access_token @@ -88,10 +90,6 @@ module API find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes) end - def current_user - @current_user - end - private def find_user_by_authentication_token(token_string) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 49e659d3d27..abbe2e9ba3e 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module API module Helpers include Gitlab::Utils @@ -33,6 +32,7 @@ module API end end + # rubocop:disable Cop/ModuleWithInstanceVariables def current_user return @current_user if defined?(@current_user) @@ -396,6 +396,7 @@ module API warden.try(:authenticate) if verified_request? end + # rubocop:disable Cop/ModuleWithInstanceVariables def initial_current_user return @initial_current_user if defined?(@initial_current_user) Gitlab::Auth::UniqueIpsLimiter.limit_user! do @@ -411,6 +412,7 @@ module API end end + # rubocop:disable Cop/ModuleWithInstanceVariables def sudo! return unless sudo_identifier return unless initial_current_user diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index a187517a66a..6bb85dd2619 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module API module Helpers module InternalHelpers @@ -7,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, diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 1b21594487d..282af32ca94 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -21,7 +21,6 @@ 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 f3e5b1c1109..9e01eed06f3 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -1,6 +1,5 @@ # 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) @@ -38,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 = ['', ''] @@ -105,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"] @@ -133,6 +134,7 @@ module ExtractsPath render_404 end + # rubocop:disable Cop/ModuleWithInstanceVariables def tree @tree ||= @repo.tree(@commit.id, @path) end @@ -146,6 +148,7 @@ module ExtractsPath id end + # rubocop:disable Cop/ModuleWithInstanceVariables def ref_names return [] unless @project diff --git a/lib/gitlab/ci/charts.rb b/lib/gitlab/ci/charts.rb index a7040a8fa03..fe2fd08a67a 100644 --- a/lib/gitlab/ci/charts.rb +++ b/lib/gitlab/ci/charts.rb @@ -10,7 +10,6 @@ module Gitlab .transform_keys { |date| date.strftime(@format) } end - # rubocop:disable Cop/ModuleWithInstanceVariables def interval_step @interval_step ||= 1.day end @@ -30,7 +29,6 @@ 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 e34f4c9e101..2c96e5f65d7 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -13,7 +13,6 @@ module Gitlab # script: ... # artifacts: ... # - # rubocop:disable Cop/ModuleWithInstanceVariables module Configurable extend ActiveSupport::Concern @@ -25,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 524d349c094..1850c652c09 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Ci class Config @@ -13,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/ci/model.rb b/lib/gitlab/ci/model.rb index 213301d245e..3994a50772b 100644 --- a/lib/gitlab/ci/model.rb +++ b/lib/gitlab/ci/model.rb @@ -5,7 +5,6 @@ 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/cycle_analytics/base_query.rb b/lib/gitlab/cycle_analytics/base_query.rb index 3f6fb227a67..52fdae84c58 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -7,11 +7,11 @@ module Gitlab private - # rubocop:disable Cop/ModuleWithInstanceVariables def base_query @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/production_helper.rb b/lib/gitlab/cycle_analytics/production_helper.rb index cd7ee39d9ca..9a05c910ba0 100644 --- a/lib/gitlab/cycle_analytics/production_helper.rb +++ b/lib/gitlab/cycle_analytics/production_helper.rb @@ -1,7 +1,7 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables 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/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index f077d64d75e..32c5caf93e8 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -12,7 +12,6 @@ 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 c8689d85c0c..89cf659bce4 100644 --- a/lib/gitlab/emoji.rb +++ b/lib/gitlab/emoji.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Emoji extend self @@ -32,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) @@ -57,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/identifier.rb b/lib/gitlab/identifier.rb index 54fd9d15e7b..94678b6ec40 100644 --- a/lib/gitlab/identifier.rb +++ b/lib/gitlab/identifier.rb @@ -52,7 +52,6 @@ module Gitlab end end - # rubocop:disable Cop/ModuleWithInstanceVariables def identification_cache @identification_cache ||= { email: {}, diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index 4d0f79ef163..c4dc061eda1 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Metrics module InfluxDb @@ -150,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 476aad2f4dd..b5f9dafccab 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/lib/gitlab/metrics/prometheus.rb @@ -1,6 +1,5 @@ require 'prometheus/client' -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module Metrics module Prometheus @@ -14,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/prometheus/additional_metrics_parser.rb b/lib/gitlab/prometheus/additional_metrics_parser.rb index 37112ca3cdb..cb95daf2260 100644 --- a/lib/gitlab/prometheus/additional_metrics_parser.rb +++ b/lib/gitlab/prometheus/additional_metrics_parser.rb @@ -26,7 +26,6 @@ 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 6e377a24e57..7ac6162b54d 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -56,7 +56,6 @@ 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 fdd5c86c698..58f6245579a 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -1,4 +1,3 @@ -# 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 2cec307867c..5aaf3655396 100644 --- a/lib/gitlab/slash_commands/presenters/issue_base.rb +++ b/lib/gitlab/slash_commands/presenters/issue_base.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab module SlashCommands module Presenters @@ -11,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/gitlab/themes.rb b/lib/gitlab/themes.rb index 7805f89831b..d43eff5ba4a 100644 --- a/lib/gitlab/themes.rb +++ b/lib/gitlab/themes.rb @@ -72,7 +72,6 @@ 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 24b65630b87..2feacbb4a09 100644 --- a/lib/tasks/gitlab/task_helpers.rb +++ b/lib/tasks/gitlab/task_helpers.rb @@ -1,6 +1,5 @@ require 'rainbow/ext/string' -# rubocop:disable Cop/ModuleWithInstanceVariables module Gitlab TaskFailedError = Class.new(StandardError) TaskAbortedByUserError = Class.new(StandardError) @@ -105,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? @@ -112,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/qa/qa/runtime/namespace.rb b/qa/qa/runtime/namespace.rb index a2480569a1e..e4910b63a14 100644 --- a/qa/qa/runtime/namespace.rb +++ b/qa/qa/runtime/namespace.rb @@ -3,7 +3,6 @@ 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 index 6ed1b986fdd..95e612b58e8 100644 --- a/rubocop/cop/module_with_instance_variables.rb +++ b/rubocop/cop/module_with_instance_variables.rb @@ -45,11 +45,29 @@ module RuboCop def check_method_definition(node) node.each_child_node(:def) do |definition| - definition.each_descendant(:ivar, :ivasgn) do |offense| - add_offense(offense, :expression) + # 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/spec/rubocop/cop/module_with_instance_variables_spec.rb b/spec/rubocop/cop/module_with_instance_variables_spec.rb index ce2e156e423..bac39117dba 100644 --- a/spec/rubocop/cop/module_with_instance_variables_spec.rb +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -19,12 +19,20 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do 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 + @f = true end end RUBY @@ -59,7 +67,7 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do module N module M def f - @f ||= true + @f = true end def g @@ -79,39 +87,86 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do 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) + context 'with regular ivar assignment' do + let(:source) do + <<~RUBY module M def f @f = true end end RUBY + end - expect(cop.offenses).to be_empty + 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 From 961b0849e5098dae74050f6c49ebf3011ce072b7 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Sep 2017 18:33:47 +0800 Subject: [PATCH 03/16] Fix grammar: judge -> judgement --- doc/development/module_with_instance_variables.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 9fc28d4c343..b663782cd04 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -72,8 +72,8 @@ end ``` Unfortunately it's not easy to code more complex rules into the cop, so -we rely on people's best judge. If we could find another good pattern we -could easily add to the cop, we should do it. +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 From f8b681f6e985d49b39d399d60666b051a60a6502 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 6 Nov 2017 22:40:19 +0800 Subject: [PATCH 04/16] WIP --- app/controllers/concerns/boards_responses.rb | 6 ++---- app/controllers/concerns/creates_commit.rb | 9 +++++---- app/controllers/concerns/issuable_actions.rb | 6 ++---- .../concerns/issuable_collections.rb | 2 ++ app/controllers/concerns/issues_action.rb | 1 + .../concerns/merge_requests_action.rb | 1 + app/controllers/concerns/milestone_actions.rb | 10 ++++++---- app/models/concerns/resolvable_discussion.rb | 18 +++++------------- lib/gitlab/ci/pipeline/chain/helpers.rb | 4 ++++ 9 files changed, 28 insertions(+), 29 deletions(-) diff --git a/app/controllers/concerns/boards_responses.rb b/app/controllers/concerns/boards_responses.rb index 058e4591770..2246ad7a4e6 100644 --- a/app/controllers/concerns/boards_responses.rb +++ b/app/controllers/concerns/boards_responses.rb @@ -23,14 +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) + respond_with(@boards) # rubocop:disable Cop/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables def respond_with_board - respond_with(@board) + respond_with(@board) # rubocop:disable Cop/ModuleWithInstanceVariables end def respond_with(resource) diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 0350c9228c9..27f77af71d1 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -46,6 +46,7 @@ module CreatesCommit end end end + # rubocop:enable Cop/ModuleWithInstanceVariables def authorize_edit_tree! return if can_collaborate_with_project? @@ -90,6 +91,7 @@ module CreatesCommit } ) end + # rubocop:enable Cop/ModuleWithInstanceVariables def existing_merge_request_path project_merge_request_path(@project, @merge_request) @@ -102,18 +104,17 @@ module CreatesCommit @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened .find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch) end + # rubocop:enable Cop/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables def different_project? - @project_to_commit_into != @project + @project_to_commit_into != @project # rubocop:disable Cop/ModuleWithInstanceVariables 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, # we don't want to create a merge request. params[:create_merge_request].present? && - (different_project? || @start_branch != @branch_name) + (different_project? || @start_branch != @branch_name) # rubocop:disable Cop/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 01645d4f6c1..1916020bdd9 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -99,9 +99,8 @@ module IssuableActions end end - # rubocop:disable Cop/ModuleWithInstanceVariables def labels - @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute + @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Cop/ModuleWithInstanceVariables end def authorize_destroy_issuable! @@ -110,9 +109,8 @@ module IssuableActions end end - # rubocop:disable Cop/ModuleWithInstanceVariables def authorize_admin_issuable! - unless can?(current_user, :"admin_#{resource_name}", @project) + unless can?(current_user, :"admin_#{resource_name}", @project) # rubocop:disable Cop/ModuleWithInstanceVariables return access_denied! end end diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index cfd1d077fe8..521d6e8eca5 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -26,6 +26,7 @@ module IssuableCollections @users = [] end + # rubocop:enable Cop/ModuleWithInstanceVariables def issues_collection issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace) @@ -110,6 +111,7 @@ module IssuableCollections @filter_params.permit(IssuableFinder::VALID_PARAMS) end + # rubocop:enable Cop/ModuleWithInstanceVariables def set_default_state params[:state] = 'opened' if params[:state].blank? diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb index a28dd376c80..b6803b14fe1 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/app/controllers/concerns/issues_action.rb @@ -18,4 +18,5 @@ module IssuesAction format.atom { render layout: 'xml.atom' } end end + # rubocop:enable Cop/ModuleWithInstanceVariables end diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb index 0f61e1ccc06..20190fa256b 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/app/controllers/concerns/merge_requests_action.rb @@ -12,6 +12,7 @@ module MergeRequestsAction @collection_type = "MergeRequest" @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type) end + # rubocop:enable Cop/ModuleWithInstanceVariables private diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index 46facd915c8..4996c6b90f3 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -1,4 +1,3 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module MilestoneActions extend ActiveSupport::Concern @@ -7,7 +6,7 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_merge_requests_tab", { - merge_requests: @milestone.sorted_merge_requests, + merge_requests: @milestone.sorted_merge_requests, # rubocop:disable Cop/ModuleWithInstanceVariables show_project_name: true }) end @@ -19,7 +18,7 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_participants_tab", { - users: @milestone.participants + users: @milestone.participants # rubocop:disable Cop/ModuleWithInstanceVariables }) end end @@ -30,7 +29,8 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_labels_tab", { - labels: @milestone.labels + labels: @milestone.labels # rubocop:disable Cop/ModuleWithInstanceVariables + }) end end @@ -44,6 +44,7 @@ module MilestoneActions } end + # rubocop:disable Cop/ModuleWithInstanceVariables def milestone_redirect_path if @project project_milestone_path(@project, @milestone) @@ -53,4 +54,5 @@ module MilestoneActions dashboard_milestone_path(@milestone.safe_title, title: @milestone.title) end end + # rubocop:enable Cop/ModuleWithInstanceVariables end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 56ba4a9a4d0..43041a707d3 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -30,36 +30,28 @@ module ResolvableDiscussion allow_nil: true end - # rubocop:disable Cop/ModuleWithInstanceVariables def resolvable? - return @resolvable if @resolvable.present? - - @resolvable = potentially_resolvable? && notes.any?(&:resolvable?) + @resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?) end - # rubocop:disable Cop/ModuleWithInstanceVariables def resolved? - return @resolved if @resolved.present? - - @resolved = resolvable? && notes.none?(&:to_be_resolved?) + @resolved ||= resolvable? && notes.none?(&:to_be_resolved?) end def first_note @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?) + @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Cop/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables def last_resolved_note return unless resolved? - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Cop/ModuleWithInstanceVariables end def resolved_notes @@ -100,7 +92,7 @@ module ResolvableDiscussion yield(notes_relation) # Set the notes array to the updated notes - @notes = notes_relation.fresh.to_a + @notes = notes_relation.fresh.to_a # rubocop:disable Cop/ModuleWithInstanceVariables self.class.memoized_values.each do |var| instance_variable_set(:"@#{var}", nil) diff --git a/lib/gitlab/ci/pipeline/chain/helpers.rb b/lib/gitlab/ci/pipeline/chain/helpers.rb index 02d81286f21..36ed87dbd32 100644 --- a/lib/gitlab/ci/pipeline/chain/helpers.rb +++ b/lib/gitlab/ci/pipeline/chain/helpers.rb @@ -3,17 +3,21 @@ module Gitlab module Pipeline module Chain module Helpers + # rubocop:disable Cop/ModuleWithInstanceVariables def branch_exists? return @is_branch if defined?(@is_branch) @is_branch = project.repository.branch_exists?(pipeline.ref) end + # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:disable Cop/ModuleWithInstanceVariables def tag_exists? return @is_tag if defined?(@is_tag) @is_tag = project.repository.tag_exists?(pipeline.ref) end + # rubocop:enable Cop/ModuleWithInstanceVariables def error(message) pipeline.errors.add(:base, message) From 9ac0c76b78cd04b2505924f003dd720a0f155959 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 17 Nov 2017 20:27:16 +0800 Subject: [PATCH 05/16] Use StrongMemoize and enable/disable cops properly --- app/controllers/concerns/creates_commit.rb | 16 ++++++--- app/controllers/concerns/group_tree.rb | 2 ++ app/controllers/concerns/issuable_actions.rb | 4 ++- .../concerns/issuable_collections.rb | 11 ++++--- app/controllers/concerns/notes_actions.rb | 33 +++++++++++-------- app/controllers/concerns/preview_markdown.rb | 2 ++ app/controllers/concerns/renders_commits.rb | 3 +- app/controllers/concerns/renders_notes.rb | 1 + app/controllers/concerns/service_params.rb | 3 +- app/controllers/concerns/snippets_actions.rb | 1 + app/controllers/concerns/spammable_actions.rb | 8 ++--- .../concerns/toggle_subscription_action.rb | 3 +- app/models/concerns/mentionable.rb | 11 ++++--- app/models/concerns/milestoneish.rb | 9 ++--- app/models/concerns/noteable.rb | 1 + app/models/concerns/relative_positioning.rb | 10 +++--- app/models/concerns/resolvable_discussion.rb | 1 - app/models/concerns/routable.rb | 13 +++----- app/models/concerns/taskable.rb | 3 +- app/models/concerns/time_trackable.rb | 19 ++++++----- app/serializers/concerns/with_pagination.rb | 2 +- .../concerns/issues/resolve_discussions.rb | 17 +++++----- app/services/spam_check_service.rb | 2 ++ app/workers/concerns/new_issuable.rb | 10 +++--- .../fix_local_cache_middleware.rb | 3 +- config/initializers/rspec_profiling.rb | 5 ++- .../rugged_use_gitlab_git_attributes.rb | 6 ++-- lib/api/helpers.rb | 10 ++++-- lib/api/helpers/internal_helpers.rb | 11 +++---- lib/extracts_path.rb | 12 +++---- lib/gitlab/cache/request_cache.rb | 7 ++-- lib/gitlab/ci/charts.rb | 3 +- lib/gitlab/ci/config/entry/configurable.rb | 7 ++-- lib/gitlab/ci/config/entry/node.rb | 6 ++++ lib/gitlab/ci/config/entry/validatable.rb | 3 +- lib/gitlab/ci/pipeline/chain/helpers.rb | 18 +++++----- lib/gitlab/current_settings.rb | 3 +- lib/gitlab/cycle_analytics/base_query.rb | 5 ++- .../cycle_analytics/production_helper.rb | 5 +-- .../v1/migration_classes.rb | 5 ++- lib/gitlab/import_export/command_line_util.rb | 3 +- lib/gitlab/metrics/influx_db.rb | 1 + lib/gitlab/metrics/prometheus.rb | 18 +++++----- lib/gitlab/path_regex.rb | 1 - .../slash_commands/presenters/issue_base.rb | 17 +++++----- lib/tasks/gettext.rake | 1 + lib/tasks/gitlab/task_helpers.rb | 19 +++++------ qa/qa/runtime/scenario.rb | 8 +++-- 48 files changed, 193 insertions(+), 169 deletions(-) diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 27f77af71d1..0f0a04a4ce1 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -1,5 +1,6 @@ module CreatesCommit extend ActiveSupport::Concern + include Gitlab::Utils::StrongMemoize # rubocop:disable Cop/ModuleWithInstanceVariables def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) @@ -94,15 +95,20 @@ module CreatesCommit # rubocop:enable Cop/ModuleWithInstanceVariables def existing_merge_request_path - project_merge_request_path(@project, @merge_request) + project_merge_request_path(@project, @merge_request) # rubocop:disable Cop/ModuleWithInstanceVariables end # rubocop:disable Cop/ModuleWithInstanceVariables def merge_request_exists? - return @merge_request if defined?(@merge_request) - - @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened - .find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch) + strong_memoize(:merge_request) do + MergeRequestsFinder.new(current_user, project_id: @project.id) + .execute + .opened + .find_by( + source_project_id: @project_to_commit_into, + source_branch: @branch_name, + target_branch: @start_branch) + end end # rubocop:enable Cop/ModuleWithInstanceVariables diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb index 9d4f97aa443..418fcc4a18f 100644 --- a/app/controllers/concerns/group_tree.rb +++ b/app/controllers/concerns/group_tree.rb @@ -1,4 +1,5 @@ module GroupTree + # rubocop:disable Cop/ModuleWithInstanceVariables def render_group_tree(groups) @groups = if params[:filter].present? Gitlab::GroupHierarchy.new(groups.search(params[:filter])) @@ -20,5 +21,6 @@ module GroupTree render json: serializer.represent(@groups) end end + # rubocop:enable Cop/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 5b9c016bb8b..b1a7a53f94a 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -142,6 +142,7 @@ module IssuableActions @resource_name ||= controller_name.singularize end + # rubocop:disable Cop/ModuleWithInstanceVariables def render_entity_json if @issuable.valid? render json: serializer.represent(@issuable) @@ -149,6 +150,7 @@ module IssuableActions render json: { errors: @issuable.errors.full_messages }, status: :unprocessable_entity end end + # rubocop:enable Cop/ModuleWithInstanceVariables def serializer raise NotImplementedError @@ -159,6 +161,6 @@ module IssuableActions end def parent - @project || @group + @project || @group # rubocop:disable Cop/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index aa5bcbef7ea..9083c2b6b5c 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -2,6 +2,7 @@ module IssuableCollections extend ActiveSupport::Concern include SortingHelper include Gitlab::IssuableMetadata + include Gitlab::Utils::StrongMemoize included do helper_method :finder @@ -43,7 +44,7 @@ module IssuableCollections def redirect_out_of_range(total_pages) return false if total_pages.zero? - out_of_range = @issuables.current_page > total_pages + out_of_range = @issuables.current_page > total_pages # rubocop:disable Cop/ModuleWithInstanceVariables if out_of_range redirect_to(url_for(params.merge(page: total_pages, only_path: true))) @@ -53,7 +54,7 @@ module IssuableCollections end def issuable_page_count - page_count_for_relation(@issuables, finder.row_count) + page_count_for_relation(@issuables, finder.row_count) # rubocop:disable Cop/ModuleWithInstanceVariables end def page_count_for_relation(relation, row_count) @@ -133,9 +134,9 @@ module IssuableCollections end def finder - return @finder if defined?(@finder) - - @finder = issuable_finder_for(@finder_type) + strong_memoize(:finder) do + issuable_finder_for(@finder_type) # rubocop:disable Cop/ModuleWithInstanceVariables + end end def collection_type diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index be153d9fdbd..e6ef1f6f5e4 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -1,6 +1,6 @@ -# rubocop:disable Cop/ModuleWithInstanceVariables module NotesActions include RendersNotes + include Gitlab::Utils::StrongMemoize extend ActiveSupport::Concern included do @@ -31,6 +31,7 @@ module NotesActions render json: notes_json end + # rubocop:disable Cop/ModuleWithInstanceVariables def create create_params = note_params.merge( merge_request_diff_head_sha: params[:merge_request_diff_head_sha], @@ -48,7 +49,9 @@ module NotesActions format.html { redirect_back_or_default } end end + # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:disable Cop/ModuleWithInstanceVariables def update @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) @@ -61,6 +64,7 @@ module NotesActions format.html { redirect_back_or_default } end end + # rubocop:enable Cop/ModuleWithInstanceVariables def destroy if note.editable? @@ -139,7 +143,7 @@ module NotesActions end else template = "discussions/_diff_discussion" - @fresh_discussion = true + @fresh_discussion = true # rubocop:disable Cop/ModuleWithInstanceVariables locals = { discussions: [discussion], on_image: on_image } end @@ -192,7 +196,7 @@ module NotesActions end def noteable - @noteable ||= notes_finder.target || @note&.noteable + @noteable ||= notes_finder.target || @note&.noteable # rubocop:disable Cop/ModuleWithInstanceVariables end def require_noteable! @@ -212,20 +216,21 @@ module NotesActions end def note_project - return @note_project if defined?(@note_project) - return nil unless project + strong_memoize(:note_project) do + return nil unless project - note_project_id = params[:note_project_id] + note_project_id = params[:note_project_id] - @note_project = - if note_project_id.present? - Project.find(note_project_id) - else - project - end + the_project = + if note_project_id.present? + Project.find(note_project_id) + else + project + end - return access_denied! unless can?(current_user, :create_note, @note_project) + return access_denied! unless can?(current_user, :create_note, the_project) - @note_project + the_project + end end end diff --git a/app/controllers/concerns/preview_markdown.rb b/app/controllers/concerns/preview_markdown.rb index 5ce602b55a8..01c95612922 100644 --- a/app/controllers/concerns/preview_markdown.rb +++ b/app/controllers/concerns/preview_markdown.rb @@ -1,6 +1,7 @@ module PreviewMarkdown extend ActiveSupport::Concern + # rubocop:disable Cop/ModuleWithInstanceVariables def preview_markdown result = PreviewMarkdownService.new(@project, current_user, params).execute @@ -19,4 +20,5 @@ module PreviewMarkdown } } end + # rubocop:enable Cop/ModuleWithInstanceVariables end diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb index 675fefd0d36..7b8cee435ee 100644 --- a/app/controllers/concerns/renders_commits.rb +++ b/app/controllers/concerns/renders_commits.rb @@ -1,7 +1,6 @@ module RendersCommits - # rubocop:disable Cop/ModuleWithInstanceVariables def prepare_commits_for_rendering(commits) - Banzai::CommitRenderer.render(commits, @project, current_user) + Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Cop/ModuleWithInstanceVariables commits end diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb index 754e88660bf..4ca6d9db5cf 100644 --- a/app/controllers/concerns/renders_notes.rb +++ b/app/controllers/concerns/renders_notes.rb @@ -8,6 +8,7 @@ module RendersNotes notes end + # rubocop:enable Cop/ModuleWithInstanceVariables private diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index ce60267f345..47b10c3ef3b 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -65,9 +65,8 @@ 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 + dynamic_params = @service.event_channel_names + @service.event_names # rubocop:disable Cop/ModuleWithInstanceVariables service_params = params.permit(:id, service: ALLOWED_PARAMS_CE + dynamic_params) if service_params[:service].is_a?(Hash) diff --git a/app/controllers/concerns/snippets_actions.rb b/app/controllers/concerns/snippets_actions.rb index 4216c1fe063..046dae480c1 100644 --- a/app/controllers/concerns/snippets_actions.rb +++ b/app/controllers/concerns/snippets_actions.rb @@ -15,6 +15,7 @@ module SnippetsActions filename: @snippet.sanitized_file_name ) end + # rubocop:enable Cop/ModuleWithInstanceVariables private diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index ef6e14c9e4c..c9cddc7a1ba 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -2,6 +2,7 @@ module SpammableActions extend ActiveSupport::Concern include Recaptcha::Verify + include Gitlab::Utils::StrongMemoize included do before_action :authorize_submit_spammable!, only: :mark_as_spam @@ -17,11 +18,10 @@ module SpammableActions private - # rubocop:disable Cop/ModuleWithInstanceVariables def ensure_spam_config_loaded! - return @spam_config_loaded if defined?(@spam_config_loaded) - - @spam_config_loaded = Gitlab::Recaptcha.load_configurations! + strong_memoize(:spam_config_loaded) do + Gitlab::Recaptcha.load_configurations! + end end def recaptcha_check_with_fallback(&fallback) diff --git a/app/controllers/concerns/toggle_subscription_action.rb b/app/controllers/concerns/toggle_subscription_action.rb index 0a6d40d36ea..776583579e8 100644 --- a/app/controllers/concerns/toggle_subscription_action.rb +++ b/app/controllers/concerns/toggle_subscription_action.rb @@ -11,9 +11,8 @@ module ToggleSubscriptionAction private - # rubocop:disable Cop/ModuleWithInstanceVariables def subscribable_project - @project || raise(NotImplementedError) + @project ||= raise(NotImplementedError) end def subscribable_resource diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 7644f2ea95f..69de1044c2d 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -43,15 +43,12 @@ module Mentionable self end - # rubocop:disable Cop/ModuleWithInstanceVariables def all_references(current_user = nil, extractor: nil) - @extractors ||= {} - # Use custom extractor if it's passed in the function parameters. if extractor - @extractors[current_user] = extractor + extractors[current_user] = extractor else - extractor = @extractors[current_user] ||= Gitlab::ReferenceExtractor.new(project, current_user) + extractor = extractors[current_user] ||= Gitlab::ReferenceExtractor.new(project, current_user) extractor.reset_memoized_values end @@ -70,6 +67,10 @@ module Mentionable extractor end + def extractors + @extractors ||= {} + end + def mentioned_users(current_user = nil) all_references(current_user).users end diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index c22fb01a4ba..fd6703831e4 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -102,11 +102,12 @@ module Milestoneish end end - # rubocop:disable Cop/ModuleWithInstanceVariables def memoize_per_user(user, method_name) - @memoized ||= {} - @memoized[method_name] ||= {} - @memoized[method_name][user&.id] ||= yield + memoized_users[method_name][user&.id] ||= yield + end + + def memoized_users + @memoized_users ||= Hash.new { |h, k| h[k] = {} } end # override in a class that includes this module to get a faster query diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index b44274f6145..fdea8bcb918 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -55,6 +55,7 @@ module Noteable discussion_notes.resolvable.discussions(self) end end + # rubocop:enable Cop/ModuleWithInstanceVariables def discussions_resolvable? resolvable_discussions.any?(&:resolvable?) diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index 9a7b9cd12b0..2ecce745ab9 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -44,7 +44,6 @@ 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 @@ -53,13 +52,12 @@ module RelativePositioning # to its predecessor. This process will recursively move all the predecessors until we have a place if (after.relative_position - before.relative_position) < 2 before.move_before - @positionable_neighbours = [before] + @positionable_neighbours = [before] # rubocop:disable Cop/ModuleWithInstanceVariables end 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 @@ -67,7 +65,7 @@ module RelativePositioning if before.shift_after? issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_after) issue_to_move.move_after - @positionable_neighbours = [issue_to_move] + @positionable_neighbours = [issue_to_move] # rubocop:disable Cop/ModuleWithInstanceVariables pos_after = issue_to_move.relative_position end @@ -75,7 +73,6 @@ 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 @@ -83,7 +80,7 @@ module RelativePositioning if after.shift_before? issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_before) issue_to_move.move_before - @positionable_neighbours = [issue_to_move] + @positionable_neighbours = [issue_to_move] # rubocop:disable Cop/ModuleWithInstanceVariables pos_before = issue_to_move.relative_position end @@ -144,4 +141,5 @@ module RelativePositioning status end + # rubocop:enable Cop/ModuleWithInstanceVariables end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 43041a707d3..fc54a8bbca0 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -84,7 +84,6 @@ 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 05ddae42d2d..efec55d7376 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -86,10 +86,9 @@ module Routable end end - # rubocop:disable Cop/ModuleWithInstanceVariables def full_name if route && route.name.present? - @full_name ||= route.name + @full_name ||= route.name # rubocop:disable Cop/ModuleWithInstanceVariables else update_route if persisted? @@ -113,7 +112,7 @@ module Routable def expires_full_path_cache RequestStore.delete(full_path_key) if RequestStore.active? - @full_path = nil + @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables end def build_full_path @@ -126,10 +125,9 @@ module Routable private - # rubocop:disable Cop/ModuleWithInstanceVariables def uncached_full_path if route && route.path.present? - @full_path ||= route.path + @full_path ||= route.path # rubocop:disable Cop/ModuleWithInstanceVariables else update_route if persisted? @@ -164,12 +162,11 @@ module Routable route.save end - # rubocop:disable Cop/ModuleWithInstanceVariables def prepare_route route || build_route(source: self) route.path = build_full_path route.name = build_full_name - @full_path = nil - @full_name = nil + @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables + @full_name = nil # rubocop:disable Cop/ModuleWithInstanceVariables end end diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index a73de49b9bb..378a3ede2aa 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -36,11 +36,10 @@ module Taskable end # Called by `TaskList::Summary` - # rubocop:disable Cop/ModuleWithInstanceVariables def task_list_items return [] if description.blank? - @task_list_items ||= Taskable.get_tasks(description) + @task_list_items ||= Taskable.get_tasks(description) # rubocop:disable Cop/ModuleWithInstanceVariables end def tasks diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index 49438908c36..f1b43cb38e1 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -36,6 +36,7 @@ module TimeTrackable end end alias_method :spend_time=, :spend_time + # rubocop:enable Cop/ModuleWithInstanceVariables def total_time_spent timelogs.sum(:time_spent) @@ -51,11 +52,11 @@ module TimeTrackable private - # rubocop:disable Cop/ModuleWithInstanceVariables def reset_spent_time - timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) + timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) # rubocop:disable Cop/ModuleWithInstanceVariables end + # rubocop:disable Cop/ModuleWithInstanceVariables def add_or_subtract_spent_time timelogs.new( time_spent: time_spent, @@ -63,17 +64,19 @@ module TimeTrackable spent_at: @spent_at ) end + # rubocop:enable Cop/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables def check_negative_time_spent return if time_spent.nil? || time_spent == :reset - # we need to cache the total time spent so multiple calls to #valid? - # doesn't give a false error - @original_total_time_spent ||= total_time_spent - - if time_spent < 0 && (time_spent.abs > @original_total_time_spent) + if time_spent < 0 && (time_spent.abs > original_total_time_spent) errors.add(:time_spent, 'Time to subtract exceeds the total time spent') end end + + # we need to cache the total time spent so multiple calls to #valid? + # doesn't give a false error + def original_total_time_spent + @original_total_time_spent ||= total_time_spent + end end diff --git a/app/serializers/concerns/with_pagination.rb b/app/serializers/concerns/with_pagination.rb index d29e22d6740..89631b73fcf 100644 --- a/app/serializers/concerns/with_pagination.rb +++ b/app/serializers/concerns/with_pagination.rb @@ -14,7 +14,7 @@ module WithPagination # we shouldn't try to paginate single resources def represent(resource, opts = {}) if paginated? && resource.respond_to?(:page) - super(@paginator.paginate(resource), opts) + super(paginator.paginate(resource), opts) else super(resource, opts) end diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb index df78ffbb6bd..fc312aad8bd 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -1,5 +1,7 @@ module Issues module ResolveDiscussions + include Gitlab::Utils::StrongMemoize + attr_reader :merge_request_to_resolve_discussions_of_iid, :discussion_to_resolve_id # rubocop:disable Cop/ModuleWithInstanceVariables @@ -7,21 +9,20 @@ module Issues @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:enable Cop/ModuleWithInstanceVariables - # 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) - - @merge_request_to_resolve_discussions_of = MergeRequestsFinder.new(current_user, project_id: project.id) - .execute - .find_by(iid: merge_request_to_resolve_discussions_of_iid) + strong_memoize(:merge_request_to_resolve_discussions_of) do + MergeRequestsFinder.new(current_user, project_id: project.id) + .execute + .find_by(iid: merge_request_to_resolve_discussions_of_iid) + end end - # rubocop:disable Cop/ModuleWithInstanceVariables def discussions_to_resolve return [] unless merge_request_to_resolve_discussions_of - @discussions_to_resolve ||= + @discussions_to_resolve ||= # rubocop:disable Cop/ModuleWithInstanceVariables if discussion_to_resolve_id discussion_or_nil = merge_request_to_resolve_discussions_of .find_discussion(discussion_to_resolve_id) diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 9b2a601f84b..31ec1e9713e 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -14,6 +14,7 @@ module SpamCheckService @recaptcha_verified = params.delete(:recaptcha_verified) @spam_log_id = params.delete(:spam_log_id) end + # rubocop:enable Cop/ModuleWithInstanceVariables # 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 @@ -26,4 +27,5 @@ module SpamCheckService user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) end end + # rubocop:enable Cop/ModuleWithInstanceVariables end diff --git a/app/workers/concerns/new_issuable.rb b/app/workers/concerns/new_issuable.rb index b6aea921aae..9a15e79d0c3 100644 --- a/app/workers/concerns/new_issuable.rb +++ b/app/workers/concerns/new_issuable.rb @@ -8,18 +8,16 @@ module NewIssuable user && issuable end - # rubocop:disable Cop/ModuleWithInstanceVariables def set_user(user_id) - @user = User.find_by(id: user_id) + @user = User.find_by(id: user_id) # rubocop:disable Cop/ModuleWithInstanceVariables - log_error(User, user_id) unless @user + log_error(User, user_id) unless @user # rubocop:disable Cop/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables def set_issuable(issuable_id) - @issuable = issuable_class.find_by(id: issuable_id) + @issuable = issuable_class.find_by(id: issuable_id) # rubocop:disable Cop/ModuleWithInstanceVariables - log_error(issuable_class, issuable_id) unless @issuable + log_error(issuable_class, issuable_id) unless @issuable # rubocop:disable Cop/ModuleWithInstanceVariables end def log_error(record_class, record_id) diff --git a/config/initializers/fix_local_cache_middleware.rb b/config/initializers/fix_local_cache_middleware.rb index c9abe0c1255..1f043408b4e 100644 --- a/config/initializers/fix_local_cache_middleware.rb +++ b/config/initializers/fix_local_cache_middleware.rb @@ -4,10 +4,9 @@ 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) + response = @app.call(env) # rubocop:disable Cop/ModuleWithInstanceVariables response[2] = ::Rack::BodyProxy.new(response[2]) do LocalCacheRegistry.set_cache_for(local_cache_key, nil) end diff --git a/config/initializers/rspec_profiling.rb b/config/initializers/rspec_profiling.rb index d6c75a611b9..c732e303f03 100644 --- a/config/initializers/rspec_profiling.rb +++ b/config/initializers/rspec_profiling.rb @@ -16,14 +16,13 @@ module RspecProfilingExt end module Run - # rubocop:disable Cop/ModuleWithInstanceVariables def example_finished(*args) super rescue => err - return if @already_logged_example_finished_error + return if @already_logged_example_finished_error # rubocop:disable Cop/ModuleWithInstanceVariables $stderr.puts "rspec_profiling couldn't collect an example: #{err}. Further warnings suppressed." - @already_logged_example_finished_error = true + @already_logged_example_finished_error = true # rubocop:disable Cop/ModuleWithInstanceVariables end alias_method :example_passed, :example_finished diff --git a/config/initializers/rugged_use_gitlab_git_attributes.rb b/config/initializers/rugged_use_gitlab_git_attributes.rb index abfa2c354cd..1cfb3bcb4bd 100644 --- a/config/initializers/rugged_use_gitlab_git_attributes.rb +++ b/config/initializers/rugged_use_gitlab_git_attributes.rb @@ -14,10 +14,12 @@ module Rugged class Repository module UseGitlabGitAttributes - # rubocop:disable Cop/ModuleWithInstanceVariables def fetch_attributes(name, *) + attributes.attributes(name) + end + + def attributes @attributes ||= Gitlab::Git::Attributes.new(path) - @attributes.attributes(name) end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 7f436b69091..c4f81443282 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -33,6 +33,10 @@ module API end # rubocop:disable Cop/ModuleWithInstanceVariables + # We can't rewrite this with StrongMemoize because `sudo!` would + # actually write to `@current_user`, and `sudo?` would immediately + # call `current_user` again which reads from `@current_user`. + # We should rewrite this in a way that using StrongMemoize is possible def current_user return @current_user if defined?(@current_user) @@ -46,6 +50,7 @@ module API @current_user end + # rubocop:enable Cop/ModuleWithInstanceVariables def sudo? initial_current_user != current_user @@ -394,6 +399,7 @@ module API private + # rubocop:disable Cop/ModuleWithInstanceVariables def initial_current_user return @initial_current_user if defined?(@initial_current_user) @@ -403,8 +409,8 @@ module API unauthorized! end end + # rubocop:enable Cop/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables def sudo! return unless sudo_identifier @@ -423,7 +429,7 @@ module API sudoed_user = find_user(sudo_identifier) not_found!("User with ID or username '#{sudo_identifier}'") unless sudoed_user - @current_user = sudoed_user + @current_user = sudoed_user # rubocop:disable Cop/ModuleWithInstanceVariables end def sudo_identifier diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 0d57c822578..e2077d33b9f 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -8,16 +8,14 @@ module API attr_reader :redirected_path - # rubocop:disable Cop/ModuleWithInstanceVariables def wiki? - set_project unless defined?(@wiki) - @wiki + set_project unless defined?(@wiki) # rubocop:disable Cop/ModuleWithInstanceVariables + @wiki # rubocop:disable Cop/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables def project - set_project unless defined?(@project) - @project + set_project unless defined?(@project) # rubocop:disable Cop/ModuleWithInstanceVariables + @project # rubocop:disable Cop/ModuleWithInstanceVariables end def ssh_authentication_abilities @@ -78,6 +76,7 @@ module API @project, @wiki, @redirected_path = Gitlab::RepoPath.parse(params[:project]) end end + # rubocop:enable Cop/ModuleWithInstanceVariables # Project id to pass between components that don't share/don't have # access to the same filesystem mounts diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index 9e01eed06f3..40a65aad631 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -37,11 +37,10 @@ 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 = ['', ''] - return pair unless @project + return pair unless @project # rubocop:disable Cop/ModuleWithInstanceVariables if id =~ /^(\h{40})(.+)/ # If the ref appears to be a SHA, we're done, just split the string @@ -133,10 +132,10 @@ module ExtractsPath rescue RuntimeError, NoMethodError, InvalidPathError render_404 end + # rubocop:enable Cop/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables def tree - @tree ||= @repo.tree(@commit.id, @path) + @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Cop/ModuleWithInstanceVariables end private @@ -148,10 +147,9 @@ module ExtractsPath id end - # rubocop:disable Cop/ModuleWithInstanceVariables def ref_names - return [] unless @project + return [] unless @project # rubocop:disable Cop/ModuleWithInstanceVariables - @ref_names ||= @project.repository.ref_names + @ref_names ||= @project.repository.ref_names # rubocop:disable Cop/ModuleWithInstanceVariables end end diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb index 65e3e672894..ecc85f847d4 100644 --- a/lib/gitlab/cache/request_cache.rb +++ b/lib/gitlab/cache/request_cache.rb @@ -45,12 +45,13 @@ module Gitlab klass.prepend(extension) end - # rubocop:disable Cop/ModuleWithInstanceVariables + attr_accessor :request_cache_key_block + def request_cache_key(&block) if block_given? - @request_cache_key = block + self.request_cache_key_block = block else - @request_cache_key + request_cache_key_block end end diff --git a/lib/gitlab/ci/charts.rb b/lib/gitlab/ci/charts.rb index fe2fd08a67a..e94166aee0c 100644 --- a/lib/gitlab/ci/charts.rb +++ b/lib/gitlab/ci/charts.rb @@ -2,12 +2,11 @@ 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)") .count(:created_at) - .transform_keys { |date| date.strftime(@format) } + .transform_keys { |date| date.strftime(@format) } # rubocop:disable Cop/ModuleWithInstanceVariables end def interval_step diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index 2c96e5f65d7..63b803c15af 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -24,21 +24,20 @@ module Gitlab end end - # rubocop:disable Cop/ModuleWithInstanceVariables def compose!(deps = nil) return unless valid? self.class.nodes.each do |key, factory| factory - .value(@config[key]) + .value(config[key]) .with(key: key, parent: self) - @entries[key] = factory.create! + entries[key] = factory.create! end yield if block_given? - @entries.each_value do |entry| + entries.each_value do |entry| entry.compose!(deps) end end diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb index c868943c42e..1fba0b2db0b 100644 --- a/lib/gitlab/ci/config/entry/node.rb +++ b/lib/gitlab/ci/config/entry/node.rb @@ -90,6 +90,12 @@ module Gitlab def self.aspects @aspects ||= [] end + + private + + def entries + @entries + end end end end diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index 1850c652c09..0dc359a86c3 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -12,9 +12,8 @@ module Gitlab end end - # rubocop:disable Cop/ModuleWithInstanceVariables def errors - @validator.messages + descendants.flat_map(&:errors) + @validator.messages + descendants.flat_map(&:errors) # rubocop:disable Cop/ModuleWithInstanceVariables end class_methods do diff --git a/lib/gitlab/ci/pipeline/chain/helpers.rb b/lib/gitlab/ci/pipeline/chain/helpers.rb index 36ed87dbd32..7ab7a64c7e3 100644 --- a/lib/gitlab/ci/pipeline/chain/helpers.rb +++ b/lib/gitlab/ci/pipeline/chain/helpers.rb @@ -3,21 +3,19 @@ module Gitlab module Pipeline module Chain module Helpers - # rubocop:disable Cop/ModuleWithInstanceVariables + include Gitlab::Utils::StrongMemoize + def branch_exists? - return @is_branch if defined?(@is_branch) - - @is_branch = project.repository.branch_exists?(pipeline.ref) + strong_memoize(:is_branch) do + project.repository.branch_exists?(pipeline.ref) + end end - # rubocop:enable Cop/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables def tag_exists? - return @is_tag if defined?(@is_tag) - - @is_tag = project.repository.tag_exists?(pipeline.ref) + strong_memoize(:is_tag) do + project.repository.tag_exists?(pipeline.ref) + end end - # rubocop:enable Cop/ModuleWithInstanceVariables def error(message) pipeline.errors.add(:base, message) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 4e0dadcc3c7..dfb2cfe42b7 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -52,9 +52,8 @@ 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) + @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) # rubocop:disable Cop/ModuleWithInstanceVariables rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError # In case migrations the application_settings table is not created yet, # we fallback to a simple OpenStruct diff --git a/lib/gitlab/cycle_analytics/base_query.rb b/lib/gitlab/cycle_analytics/base_query.rb index 52fdae84c58..05b4928c3b9 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -11,13 +11,12 @@ 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])) - .where(issue_table[:project_id].eq(@project.id)) + .where(issue_table[:project_id].eq(@project.id)) # rubocop:disable Cop/ModuleWithInstanceVariables .where(issue_table[:deleted_at].eq(nil)) - .where(issue_table[:created_at].gteq(@options[:from])) + .where(issue_table[:created_at].gteq(@options[:from])) # rubocop:disable Cop/ModuleWithInstanceVariables # Load merge_requests query = query.join(mr_table, Arel::Nodes::OuterJoin) diff --git a/lib/gitlab/cycle_analytics/production_helper.rb b/lib/gitlab/cycle_analytics/production_helper.rb index 9a05c910ba0..ebbff1b2f33 100644 --- a/lib/gitlab/cycle_analytics/production_helper.rb +++ b/lib/gitlab/cycle_analytics/production_helper.rb @@ -1,9 +1,10 @@ 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])) + super + .where(mr_metrics_table[:first_deployed_to_production_at] + .gteq(@options[:from])) # rubocop:disable Cop/ModuleWithInstanceVariables 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 6959fa74293..9ae1f66a182 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,11 +3,10 @@ module Gitlab module RenameReservedPathsMigration module V1 module MigrationClasses - # rubocop:disable Cop/ModuleWithInstanceVariables module Routable def full_path if route && route.path.present? - @full_path ||= route.path + @full_path ||= route.path # rubocop:disable Cop/ModuleWithInstanceVariables else update_route if persisted? @@ -31,7 +30,7 @@ module Gitlab def prepare_route route || build_route(source: self) route.path = build_full_path - @full_path = nil + @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables end end diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 5ac57c5775a..19dab0037bb 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -30,10 +30,9 @@ 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? + @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? # rubocop:disable Cop/ModuleWithInstanceVariables status.zero? end diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index 3c5f9099584..fa88d41be73 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -171,6 +171,7 @@ module Gitlab @pool end end + # rubocop:enable Cop/ModuleWithInstanceVariables end end end diff --git a/lib/gitlab/metrics/prometheus.rb b/lib/gitlab/metrics/prometheus.rb index 75461b45005..b0b8e8436db 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/lib/gitlab/metrics/prometheus.rb @@ -4,6 +4,7 @@ module Gitlab module Metrics module Prometheus include Gitlab::CurrentSettings + include Gitlab::Utils::StrongMemoize REGISTRY_MUTEX = Mutex.new PROVIDER_MUTEX = Mutex.new @@ -16,18 +17,19 @@ 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) - - @prometheus_metrics_enabled = prometheus_metrics_enabled_unmemoized + strong_memoize(:prometheus_metrics_enabled) do + prometheus_metrics_enabled_unmemoized + end end def registry - return @registry if @registry - - REGISTRY_MUTEX.synchronize do - @registry ||= ::Prometheus::Client.registry + strong_memoize(:registry) do + REGISTRY_MUTEX.synchronize do + strong_memoize(:registry) do + ::Prometheus::Client.registry + end + end end end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 9a3817ff00a..9a91f8bf96a 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -1,4 +1,3 @@ -# 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 5aaf3655396..31c1e97efba 100644 --- a/lib/gitlab/slash_commands/presenters/issue_base.rb +++ b/lib/gitlab/slash_commands/presenters/issue_base.rb @@ -10,36 +10,37 @@ module Gitlab issuable.open? ? 'Open' : 'Closed' end - # rubocop:disable Cop/ModuleWithInstanceVariables def project - @resource.project + resource.project end - # rubocop:disable Cop/ModuleWithInstanceVariables def author - @resource.author + resource.author end - # rubocop:disable Cop/ModuleWithInstanceVariables def fields [ { title: "Assignee", - value: @resource.assignees.any? ? @resource.assignees.first.name : "_None_", + value: resource.assignees.any? ? resource.assignees.first.name : "_None_", short: true }, { title: "Milestone", - value: @resource.milestone ? @resource.milestone.title : "_None_", + value: resource.milestone ? resource.milestone.title : "_None_", short: true }, { title: "Labels", - value: @resource.labels.any? ? @resource.label_names.join(', ') : "_None_", + value: resource.labels.any? ? resource.label_names.join(', ') : "_None_", short: true } ] end + + private + + attr_reader :resource end end end diff --git a/lib/tasks/gettext.rake b/lib/tasks/gettext.rake index 35ba729c156..247d7be7d78 100644 --- a/lib/tasks/gettext.rake +++ b/lib/tasks/gettext.rake @@ -23,6 +23,7 @@ namespace :gettext do desc 'Lint all po files in `locale/' task lint: :environment do require 'simple_po_parser' + require 'gitlab/utils' FastGettext.silence_errors files = Dir.glob(Rails.root.join('locale/*/gitlab.po')) diff --git a/lib/tasks/gitlab/task_helpers.rb b/lib/tasks/gitlab/task_helpers.rb index 2feacbb4a09..6723662703c 100644 --- a/lib/tasks/gitlab/task_helpers.rb +++ b/lib/tasks/gitlab/task_helpers.rb @@ -1,10 +1,13 @@ require 'rainbow/ext/string' +require 'gitlab/utils/strong_memoize' module Gitlab TaskFailedError = Class.new(StandardError) TaskAbortedByUserError = Class.new(StandardError) module TaskHelpers + include Gitlab::Utils::StrongMemoize + extend self # Ask if the user wants to continue @@ -104,19 +107,17 @@ module Gitlab Gitlab.config.gitlab.user end - # rubocop:disable Cop/ModuleWithInstanceVariables def gitlab_user? - return @is_gitlab_user unless @is_gitlab_user.nil? - - current_user = run_command(%w(whoami)).chomp - @is_gitlab_user = current_user == gitlab_user + strong_memoize(:is_gitlab_user) do + current_user = run_command(%w(whoami)).chomp + current_user == gitlab_user + end end - # rubocop:disable Cop/ModuleWithInstanceVariables def warn_user_is_not_gitlab - return if @warned_user_not_gitlab + return if gitlab_user? - unless gitlab_user? + strong_memoize(:warned_user_not_gitlab) do current_user = run_command(%w(whoami)).chomp puts " Warning ".color(:black).background(:yellow) @@ -124,8 +125,6 @@ module Gitlab puts " Things may work\/fail for the wrong reasons." puts " For correct results you should run this as user #{gitlab_user.color(:magenta)}." puts "" - - @warned_user_not_gitlab = true end end diff --git a/qa/qa/runtime/scenario.rb b/qa/qa/runtime/scenario.rb index 7ef59046640..15d4112d347 100644 --- a/qa/qa/runtime/scenario.rb +++ b/qa/qa/runtime/scenario.rb @@ -6,13 +6,15 @@ module QA module Scenario extend self - attr_reader :attributes + def attributes + @attributes ||= {} + end def define(attribute, value) - (@attributes ||= {}).store(attribute.to_sym, value) + attributes.store(attribute.to_sym, value) define_singleton_method(attribute) do - @attributes[attribute.to_sym].tap do |value| + attributes[attribute.to_sym].tap do |value| if value.to_s.empty? raise ArgumentError, "Empty `#{attribute}` attribute!" end From 7441c7af9acb849ba5f6a25895614fe5cc8023b2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 17 Nov 2017 21:25:49 +0800 Subject: [PATCH 06/16] Allow initialize method and single ivar --- rubocop/cop/module_with_instance_variables.rb | 20 +++++++++++-- .../module_with_instance_variables_spec.rb | 29 +++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb index 95e612b58e8..974a23bf701 100644 --- a/rubocop/cop/module_with_instance_variables.rb +++ b/rubocop/cop/module_with_instance_variables.rb @@ -46,14 +46,18 @@ module RuboCop def check_method_definition(node) node.each_child_node(:def) do |definition| # We allow this pattern: - # def f - # @f ||= true - # end + # + # 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 + # We allow initialize method and single ivar + elsif initialize_method?(definition) || single_ivar?(definition) + next else definition.each_descendant(:ivar, :ivasgn) do |offense| add_offense(offense, :expression) @@ -68,6 +72,16 @@ module RuboCop definition.child_nodes.size == 2 && node.or_asgn_type? && node.child_nodes.first.ivasgn_type? end + + def single_ivar?(definition) + node = definition.child_nodes.last + + definition.child_nodes.size == 2 && node.ivar_type? + end + + def initialize_method?(definition) + definition.children.first == :initialize + end end end end diff --git a/spec/rubocop/cop/module_with_instance_variables_spec.rb b/spec/rubocop/cop/module_with_instance_variables_spec.rb index bac39117dba..72f6b01aa81 100644 --- a/spec/rubocop/cop/module_with_instance_variables_spec.rb +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -137,6 +137,35 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do it_behaves_like 'not registering offense' end + context 'when source is using simple ivar' do + let(:source) do + <<~RUBY + module M + def f? + @f + end + end + RUBY + end + + it_behaves_like 'not registering offense' + end + + context 'when source is defining initialize' do + let(:source) do + <<~RUBY + module M + def initialize + @a = 1 + @b = 2 + 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 From ffec300b9495f0fe022e777c889407433217497e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 18 Nov 2017 00:08:47 +0800 Subject: [PATCH 07/16] Remove codes from bad merge --- rubocop/rubocop.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 7db56349cd7..65a37f578cd 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -5,8 +5,6 @@ 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' From 45568bed36095db0df94cf89a8a04458f56f33dc Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 21 Nov 2017 23:15:24 +0800 Subject: [PATCH 08/16] Updates based on feedback --- app/models/concerns/spammable.rb | 5 +- doc/development/README.md | 1 + .../module_with_instance_variables.md | 25 ++- features/support/env.rb | 4 +- lib/gitlab/ci/config/entry/configurable.rb | 6 +- rubocop/cop/module_with_instance_variables.rb | 6 +- .../module_with_instance_variables_spec.rb | 178 +++++++++--------- 7 files changed, 106 insertions(+), 119 deletions(-) diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 8b56894ec3a..5e4274619c4 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -12,6 +12,7 @@ module Spammable attr_accessor :spam attr_accessor :spam_log + alias_method :spam?, :spam after_validation :check_for_spam, on: [:create, :update] @@ -34,10 +35,6 @@ module Spammable end end - def spam? - spam - end - def check_for_spam error_msg = if Gitlab::Recaptcha.enabled? "Your #{spammable_entity_type} has been recognized as spam. "\ diff --git a/doc/development/README.md b/doc/development/README.md index 6892838be7f..c31e665e91a 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -36,6 +36,7 @@ comments: false - [`Gemfile` guidelines](gemfile.md) - [Sidekiq debugging](sidekiq_debugging.md) - [Gotchas](gotchas.md) to avoid +- [Avoid modules with instance variables](module_with_instance_variables.md) if possible - [Issue and merge requests state models](object_state_models.md) - [How to dump production data to staging](db_dump.md) - [Working with the GitHub importer](github_importer.md) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index b663782cd04..43ec8911b81 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -55,10 +55,9 @@ they communicate in a clear way, rather than implicit dependencies. ### Acceptable use -However, it's not all that bad when using instance variables in a module, -as long as it's contained in the same module, that is no other modules or -objects are touching them. If that's the case, then it would be an acceptable -use. +However, it's not always bad to use instance variables in a module, +as long as it's contained in the same module; that is, no other modules or +objects are touching them, then it would be an acceptable use. We especially allow the case where a single instance variable is used with `||=` to setup the value. This would look like: @@ -93,7 +92,7 @@ module Gitlab end ``` -It's still offending because it's not just `||=`, but We could split this +It's still offending because it's not just `||=`, but we could split this method into two: ``` ruby @@ -135,7 +134,7 @@ end ``` There are several implicit dependencies here. First, `params` should be -defined before using. Second, `filter_spam_check_params` should be called +defined before use. Second, `filter_spam_check_params` should be called before `spam_check`. These are all implicit and the includer could be using those instance variables without awareness. @@ -175,18 +174,18 @@ end ``` This way, all those instance variables are isolated in `SpamCheckService` -rather than who ever include the module, and those modules which were also -included, making it much easier to track down the issues if there's any, -and it also reduces the chance of having name conflicts. +rather than whatever includes the module, and those modules which were also +included, making it much easier to track down any issues, +and reducing the chance of having name conflicts. ### Things we might need to ignore right now -Since the way how Rails helpers and mailers work, we might not be able to +Because of the way Rails helpers and mailers work, we might not be able to avoid the use of instance variables there. For those cases, we could ignore them at the moment. At least we're not going to share those modules with -other random objects, so they're still somehow isolated. +other random objects, so they're still somewhat isolated. -### Instance variables in the views +### Instance variables in views They're terrible, because they're also shared between different controllers, and it's very hard to track where those instance variables were set when we @@ -210,5 +209,5 @@ And in the partial: - project = local_assigns.fetch(:project) ``` -This way it's very clear where those values were coming from. In the future, +This way it's clearer where those values were coming from. In the future, we should also forbid the use of instance variables in partials. diff --git a/features/support/env.rb b/features/support/env.rb index b93ddecdced..a1413522f3a 100644 --- a/features/support/env.rb +++ b/features/support/env.rb @@ -42,11 +42,11 @@ module StdoutReporterWithScenarioLocation # Override the standard reporter to show filename and line number next to each # scenario for easy, focused re-runs def before_scenario_run(scenario, step_definitions = nil) - max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? + @max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? # rubocop:disable Cop/ModuleWithInstanceVariables name = scenario.name # This number has no significance, it's just to line things up - max_length = max_step_name_length + 19 + max_length = @max_step_name_length + 19 # rubocop:disable Cop/ModuleWithInstanceVariables out.puts "\n #{'Scenario:'.green} #{name.light_green.ljust(max_length)}" \ " # #{scenario.feature.filename}:#{scenario.line}" end diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index 63b803c15af..db47c2f6185 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -59,13 +59,13 @@ module Gitlab def helpers(*nodes) nodes.each do |symbol| define_method("#{symbol}_defined?") do - @entries[symbol]&.specified? + entries[symbol]&.specified? end define_method("#{symbol}_value") do - return unless @entries[symbol] && @entries[symbol].valid? + return unless entries[symbol] && entries[symbol].valid? - @entries[symbol].value + entries[symbol].value end end end diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb index 974a23bf701..9f9856e308b 100644 --- a/rubocop/cop/module_with_instance_variables.rb +++ b/rubocop/cop/module_with_instance_variables.rb @@ -5,7 +5,7 @@ module RuboCop Do not use instance variables in a module. Please read this for the rationale behind it: - doc/development/module_with_instance_variables.md + https://docs.gitlab.com/ee/development/module_with_instance_variables.html If you think the use for this is fine, please just add: # rubocop:disable Cop/ModuleWithInstanceVariables @@ -56,9 +56,7 @@ module RuboCop add_offense(offense, :expression) end # We allow initialize method and single ivar - elsif initialize_method?(definition) || single_ivar?(definition) - next - else + elsif !initialize_method?(definition) && !single_ivar?(definition) definition.each_descendant(:ivar, :ivasgn) do |offense| add_offense(offense, :expression) end diff --git a/spec/rubocop/cop/module_with_instance_variables_spec.rb b/spec/rubocop/cop/module_with_instance_variables_spec.rb index 72f6b01aa81..df5e2dd2f04 100644 --- a/spec/rubocop/cop/module_with_instance_variables_spec.rb +++ b/spec/rubocop/cop/module_with_instance_variables_spec.rb @@ -8,7 +8,9 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do subject(:cop) { described_class.new } - shared_examples('registering offense') do + shared_examples('registering offense') do |options| + let(:offending_lines) { options[:offending_lines] } + it 'registers an offense when instance variable is used in a module' do inspect_source(cop, source) @@ -28,63 +30,57 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do end context 'when source is a regular module' do - let(:source) do - <<~RUBY - module M - def f - @f = true + it_behaves_like 'registering offense', offending_lines: [3] do + let(:source) do + <<~RUBY + module M + def f + @f = true + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3] } - - it_behaves_like 'registering offense' end context 'when source is a nested module' do - let(:source) do - <<~RUBY - module N - module M - def f - @f = true + it_behaves_like 'registering offense', offending_lines: [4] do + let(:source) do + <<~RUBY + module N + module M + def f + @f = true + end end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [4] } - - it_behaves_like 'registering offense' end context 'when source is a nested module with multiple offenses' do - let(:source) do - <<~RUBY - module N - module M - def f - @f = true - end + it_behaves_like 'registering offense', offending_lines: [4, 12] do + let(:source) do + <<~RUBY + module N + module M + def f + @f = true + end - def g - true - end + def g + true + end - def h - @h = true + def h + @h = true + end end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [4, 12] } - - it_behaves_like 'registering offense' end context 'with regular ivar assignment' do @@ -124,78 +120,74 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do end context 'when source is using simple or ivar assignment' do - let(:source) do - <<~RUBY - module M - def f - @f ||= true + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def f + @f ||= true + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is using simple ivar' do - let(:source) do - <<~RUBY - module M - def f? - @f + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def f? + @f + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is defining initialize' do - let(:source) do - <<~RUBY - module M - def initialize - @a = 1 - @b = 2 + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + module M + def initialize + @a = 1 + @b = 2 + end end - end - RUBY + RUBY + end end - - it_behaves_like 'not registering offense' end context 'when source is using simple or ivar assignment with other ivar' do - let(:source) do - <<~RUBY - module M - def f - @f ||= g(@g) + it_behaves_like 'registering offense', offending_lines: [3] do + let(:source) do + <<~RUBY + module M + def f + @f ||= g(@g) + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3] } - - it_behaves_like 'registering offense' end context 'when source is using or ivar assignment with something else' do - let(:source) do - <<~RUBY - module M - def f - @f ||= true - @f.to_s + it_behaves_like 'registering offense', offending_lines: [3, 4] do + let(:source) do + <<~RUBY + module M + def f + @f ||= true + @f.to_s + end end - end - RUBY + RUBY + end end - - let(:offending_lines) { [3, 4] } - - it_behaves_like 'registering offense' end end From 0b6d01ed775e957161e1304de44d0bd5251f4098 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 00:15:52 +0800 Subject: [PATCH 09/16] Just define allowed_ids and override it with empty value if not needed for those sub-classes. --- lib/gitlab/cycle_analytics/base_event_fetcher.rb | 4 ---- lib/gitlab/cycle_analytics/code_event_fetcher.rb | 4 ---- lib/gitlab/cycle_analytics/issue_event_fetcher.rb | 4 ---- lib/gitlab/cycle_analytics/plan_event_fetcher.rb | 4 ++++ lib/gitlab/cycle_analytics/review_event_fetcher.rb | 4 ---- lib/gitlab/cycle_analytics/staging_event_fetcher.rb | 4 ++++ spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb | 2 ++ 7 files changed, 10 insertions(+), 16 deletions(-) diff --git a/lib/gitlab/cycle_analytics/base_event_fetcher.rb b/lib/gitlab/cycle_analytics/base_event_fetcher.rb index bec201f309c..62fc3ee3b5f 100644 --- a/lib/gitlab/cycle_analytics/base_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/base_event_fetcher.rb @@ -56,10 +56,6 @@ module Gitlab end def allowed_ids - 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) diff --git a/lib/gitlab/cycle_analytics/code_event_fetcher.rb b/lib/gitlab/cycle_analytics/code_event_fetcher.rb index 39df80352b6..06357c9b377 100644 --- a/lib/gitlab/cycle_analytics/code_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/code_event_fetcher.rb @@ -19,10 +19,6 @@ module Gitlab AnalyticsMergeRequestSerializer.new(project: @project).represent(event) end - def allowed_ids - load_allowed_ids - end - def allowed_ids_finder_class MergeRequestsFinder end diff --git a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb index cc79e2dfe88..1754f91dccb 100644 --- a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb @@ -17,10 +17,6 @@ module Gitlab AnalyticsIssueSerializer.new(project: @project).represent(event) end - def allowed_ids - load_allowed_ids - end - def allowed_ids_finder_class IssuesFinder end diff --git a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb index 2479b4a7706..ca2742f2aae 100644 --- a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb @@ -19,6 +19,10 @@ module Gitlab private + def allowed_ids + nil + end + def merge_request_diff_commits @merge_request_diff_commits ||= MergeRequestDiffCommit diff --git a/lib/gitlab/cycle_analytics/review_event_fetcher.rb b/lib/gitlab/cycle_analytics/review_event_fetcher.rb index 5a7f1eb00b3..dada819a2a8 100644 --- a/lib/gitlab/cycle_analytics/review_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/review_event_fetcher.rb @@ -18,10 +18,6 @@ module Gitlab AnalyticsMergeRequestSerializer.new(project: @project).represent(event) end - def allowed_ids - load_allowed_ids - end - def allowed_ids_finder_class MergeRequestsFinder end diff --git a/lib/gitlab/cycle_analytics/staging_event_fetcher.rb b/lib/gitlab/cycle_analytics/staging_event_fetcher.rb index 36c0260dbfe..2f014153ca5 100644 --- a/lib/gitlab/cycle_analytics/staging_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/staging_event_fetcher.rb @@ -22,6 +22,10 @@ module Gitlab private + def allowed_ids + nil + end + def serialize(event) AnalyticsBuildSerializer.new.represent(event['build']) end diff --git a/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb b/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb index 0560c47f03f..3fe0493ed9b 100644 --- a/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb @@ -23,6 +23,8 @@ describe Gitlab::CycleAnalytics::BaseEventFetcher do allow_any_instance_of(described_class).to receive(:serialize) do |event| event end + allow_any_instance_of(described_class) + .to receive(:allowed_ids).and_return(nil) stub_const('Gitlab::CycleAnalytics::BaseEventFetcher::MAX_EVENTS', max_events) From 15edf741a14a53443fb39ecb59888e75697b9c2b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 00:59:16 +0800 Subject: [PATCH 10/16] Explain how to disable it in the doc --- .../module_with_instance_variables.md | 29 +++++++++++++++++++ rubocop/cop/module_with_instance_variables.rb | 3 -- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 43ec8911b81..1c64b35b782 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -178,6 +178,35 @@ rather than whatever includes the module, and those modules which were also included, making it much easier to track down any issues, and reducing the chance of having name conflicts. +### How to disable this cop + +Put the disabling comment right after your code in the same line: + +``` ruby +module M + def violating_method + @f + @g # rubocop:disable Cop/ModuleWithInstanceVariables + end +end +``` + +If there are multiple lines, you could also enable and disable for a section: + +``` ruby +module M + # rubocop:disable Cop/ModuleWithInstanceVariables + def violating_method + @f = 0 + @g = 1 + @h = 2 + end + # rubocop:enable Cop/ModuleWithInstanceVariables +end +``` + +Note that you need to enable it at some point, otherwise everything below +won't be checked. + ### Things we might need to ignore right now Because of the way Rails helpers and mailers work, we might not be able to diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb index 9f9856e308b..f101ae09ad2 100644 --- a/rubocop/cop/module_with_instance_variables.rb +++ b/rubocop/cop/module_with_instance_variables.rb @@ -6,9 +6,6 @@ module RuboCop for the rationale behind it: https://docs.gitlab.com/ee/development/module_with_instance_variables.html - - If you think the use for this is fine, please just add: - # rubocop:disable Cop/ModuleWithInstanceVariables EOL def on_module(node) From 07d3d44775f6cc5b7a1b768cb4e5b7900d543815 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 15:50:36 +0800 Subject: [PATCH 11/16] Move ModuleWithInstanceVariables to Gitlab namespace And use .rubocop.yml to exclude paths we don't care, rather than using the cop itself to exclude. --- .rubocop.yml | 15 +++- app/controllers/concerns/boards_responses.rb | 4 +- app/controllers/concerns/creates_commit.rb | 18 ++-- app/controllers/concerns/group_tree.rb | 4 +- app/controllers/concerns/issuable_actions.rb | 14 ++-- .../concerns/issuable_collections.rb | 14 ++-- app/controllers/concerns/issues_action.rb | 4 +- .../concerns/merge_requests_action.rb | 4 +- app/controllers/concerns/milestone_actions.rb | 10 +-- app/controllers/concerns/notes_actions.rb | 12 +-- app/controllers/concerns/preview_markdown.rb | 4 +- app/controllers/concerns/renders_commits.rb | 2 +- app/controllers/concerns/renders_notes.rb | 4 +- app/controllers/concerns/service_params.rb | 2 +- app/controllers/concerns/snippets_actions.rb | 4 +- app/models/concerns/noteable.rb | 4 +- app/models/concerns/relative_positioning.rb | 10 +-- app/models/concerns/resolvable_discussion.rb | 6 +- app/models/concerns/routable.rb | 10 +-- app/models/concerns/taskable.rb | 2 +- app/models/concerns/time_trackable.rb | 10 +-- .../concerns/issues/resolve_discussions.rb | 6 +- app/services/spam_check_service.rb | 8 +- app/workers/concerns/new_issuable.rb | 8 +- .../fix_local_cache_middleware.rb | 2 +- config/initializers/rspec_profiling.rb | 4 +- .../module_with_instance_variables.md | 6 +- features/support/env.rb | 4 +- lib/api/helpers.rb | 10 +-- lib/api/helpers/internal_helpers.rb | 12 +-- lib/extracts_path.rb | 12 +-- lib/gitlab/ci/charts.rb | 2 +- lib/gitlab/ci/config/entry/validatable.rb | 2 +- lib/gitlab/current_settings.rb | 2 +- lib/gitlab/cycle_analytics/base_query.rb | 4 +- .../cycle_analytics/production_helper.rb | 2 +- .../v1/migration_classes.rb | 4 +- lib/gitlab/import_export/command_line_util.rb | 2 +- lib/gitlab/metrics/influx_db.rb | 4 +- .../gitlab/module_with_instance_variables.rb | 63 ++++++++++++++ rubocop/cop/module_with_instance_variables.rb | 82 ------------------- rubocop/rubocop.rb | 2 +- .../module_with_instance_variables_spec.rb | 40 +-------- 43 files changed, 198 insertions(+), 240 deletions(-) create mode 100644 rubocop/cop/gitlab/module_with_instance_variables.rb delete mode 100644 rubocop/cop/module_with_instance_variables.rb rename spec/rubocop/cop/{ => gitlab}/module_with_instance_variables_spec.rb (77%) diff --git a/.rubocop.yml b/.rubocop.yml index c427f219a0d..d103a14518f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1185,7 +1185,20 @@ RSpec/SubjectStub: RSpec/VerifiedDoubles: Enabled: false -# GitlabSecurity ############################################################## +# Gitlab ################################################################### + +Gitlab/ModuleWithInstanceVariables: + Enable: true + Exclude: + # We ignore Rails helpers right now because it's hard to workaround it + - app/helpers/*_helper.rb + # We ignore Rails mailers right now because it's hard to workaround it + - app/mailers/emails/*.rb + # We ignore spec helpers because it usually doesn't matter + - spec/support/**/*.rb + - features/steps/**/*.rb + +# GitlabSecurity ########################################################### GitlabSecurity/DeepMunge: Enabled: true diff --git a/app/controllers/concerns/boards_responses.rb b/app/controllers/concerns/boards_responses.rb index 2246ad7a4e6..a145049dc7d 100644 --- a/app/controllers/concerns/boards_responses.rb +++ b/app/controllers/concerns/boards_responses.rb @@ -24,11 +24,11 @@ module BoardsResponses end def respond_with_boards - respond_with(@boards) # rubocop:disable Cop/ModuleWithInstanceVariables + respond_with(@boards) # rubocop:disable Gitlab/ModuleWithInstanceVariables end def respond_with_board - respond_with(@board) # rubocop:disable Cop/ModuleWithInstanceVariables + respond_with(@board) # rubocop:disable Gitlab/ModuleWithInstanceVariables end def respond_with(resource) diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 0f0a04a4ce1..6f4fdcdaa4f 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -2,7 +2,7 @@ module CreatesCommit extend ActiveSupport::Concern include Gitlab::Utils::StrongMemoize - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/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 @@ -47,7 +47,7 @@ module CreatesCommit end end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def authorize_edit_tree! return if can_collaborate_with_project? @@ -80,7 +80,7 @@ module CreatesCommit end end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def new_merge_request_path project_new_merge_request_path( @project_to_commit_into, @@ -92,13 +92,13 @@ module CreatesCommit } ) end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def existing_merge_request_path - project_merge_request_path(@project, @merge_request) # rubocop:disable Cop/ModuleWithInstanceVariables + project_merge_request_path(@project, @merge_request) # rubocop:disable Gitlab/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def merge_request_exists? strong_memoize(:merge_request) do MergeRequestsFinder.new(current_user, project_id: @project.id) @@ -110,10 +110,10 @@ module CreatesCommit target_branch: @start_branch) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def different_project? - @project_to_commit_into != @project # rubocop:disable Cop/ModuleWithInstanceVariables + @project_to_commit_into != @project # rubocop:disable Gitlab/ModuleWithInstanceVariables end def create_merge_request? @@ -121,6 +121,6 @@ module CreatesCommit # as the target branch in the same project, # we don't want to create a merge request. params[:create_merge_request].present? && - (different_project? || @start_branch != @branch_name) # rubocop:disable Cop/ModuleWithInstanceVariables + (different_project? || @start_branch != @branch_name) # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb index 418fcc4a18f..b10147835f3 100644 --- a/app/controllers/concerns/group_tree.rb +++ b/app/controllers/concerns/group_tree.rb @@ -1,5 +1,5 @@ module GroupTree - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def render_group_tree(groups) @groups = if params[:filter].present? Gitlab::GroupHierarchy.new(groups.search(params[:filter])) @@ -21,6 +21,6 @@ module GroupTree render json: serializer.represent(@groups) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index b1a7a53f94a..a725aad43e7 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -17,7 +17,7 @@ module IssuableActions end def update - @issuable = update_service.execute(issuable) # rubocop:disable Cop/ModuleWithInstanceVariables + @issuable = update_service.execute(issuable) # rubocop:disable Gitlab/ModuleWithInstanceVariables respond_to do |format| format.html do @@ -83,7 +83,7 @@ module IssuableActions def render_conflict_response respond_to do |format| format.html do - @conflict = true # rubocop:disable Cop/ModuleWithInstanceVariables + @conflict = true # rubocop:disable Gitlab/ModuleWithInstanceVariables render :edit end @@ -98,7 +98,7 @@ module IssuableActions end def labels - @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Cop/ModuleWithInstanceVariables + @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables end def authorize_destroy_issuable! @@ -108,7 +108,7 @@ module IssuableActions end def authorize_admin_issuable! - unless can?(current_user, :"admin_#{resource_name}", @project) # rubocop:disable Cop/ModuleWithInstanceVariables + unless can?(current_user, :"admin_#{resource_name}", @project) # rubocop:disable Gitlab/ModuleWithInstanceVariables return access_denied! end end @@ -142,7 +142,7 @@ module IssuableActions @resource_name ||= controller_name.singularize end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def render_entity_json if @issuable.valid? render json: serializer.represent(@issuable) @@ -150,7 +150,7 @@ module IssuableActions render json: { errors: @issuable.errors.full_messages }, status: :unprocessable_entity end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def serializer raise NotImplementedError @@ -161,6 +161,6 @@ module IssuableActions end def parent - @project || @group # rubocop:disable Cop/ModuleWithInstanceVariables + @project || @group # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 9083c2b6b5c..cfee2071472 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -10,7 +10,7 @@ module IssuableCollections private - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def set_issuables_index @issuables = issuables_collection @issuables = @issuables.page(params[:page]) @@ -35,7 +35,7 @@ module IssuableCollections @users.push(author) if author end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def issuables_collection finder.execute.preload(preload_for_collection) @@ -44,7 +44,7 @@ module IssuableCollections def redirect_out_of_range(total_pages) return false if total_pages.zero? - out_of_range = @issuables.current_page > total_pages # rubocop:disable Cop/ModuleWithInstanceVariables + out_of_range = @issuables.current_page > total_pages # rubocop:disable Gitlab/ModuleWithInstanceVariables if out_of_range redirect_to(url_for(params.merge(page: total_pages, only_path: true))) @@ -54,7 +54,7 @@ module IssuableCollections end def issuable_page_count - page_count_for_relation(@issuables, finder.row_count) # rubocop:disable Cop/ModuleWithInstanceVariables + page_count_for_relation(@issuables, finder.row_count) # rubocop:disable Gitlab/ModuleWithInstanceVariables end def page_count_for_relation(relation, row_count) @@ -69,7 +69,7 @@ module IssuableCollections finder_class.new(current_user, filter_params) end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def filter_params set_sort_order_from_cookie set_default_state @@ -94,7 +94,7 @@ module IssuableCollections @filter_params.permit(IssuableFinder::VALID_PARAMS) end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def set_default_state params[:state] = 'opened' if params[:state].blank? @@ -135,7 +135,7 @@ module IssuableCollections def finder strong_memoize(:finder) do - issuable_finder_for(@finder_type) # rubocop:disable Cop/ModuleWithInstanceVariables + issuable_finder_for(@finder_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb index 4423c7fa0aa..d4cccbe6442 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/app/controllers/concerns/issues_action.rb @@ -2,7 +2,7 @@ module IssuesAction extend ActiveSupport::Concern include IssuableCollections - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def issues @finder_type = IssuesFinder @label = finder.labels.first @@ -18,5 +18,5 @@ module IssuesAction format.atom { render layout: 'xml.atom' } end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb index de1710e7161..4d44df3bba9 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/app/controllers/concerns/merge_requests_action.rb @@ -2,7 +2,7 @@ module MergeRequestsAction extend ActiveSupport::Concern include IssuableCollections - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def merge_requests @finder_type = MergeRequestsFinder @label = finder.labels.first @@ -11,7 +11,7 @@ module MergeRequestsAction @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type) end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables private diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index 4996c6b90f3..7bec64c5d60 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -6,7 +6,7 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_merge_requests_tab", { - merge_requests: @milestone.sorted_merge_requests, # rubocop:disable Cop/ModuleWithInstanceVariables + merge_requests: @milestone.sorted_merge_requests, # rubocop:disable Gitlab/ModuleWithInstanceVariables show_project_name: true }) end @@ -18,7 +18,7 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_participants_tab", { - users: @milestone.participants # rubocop:disable Cop/ModuleWithInstanceVariables + users: @milestone.participants # rubocop:disable Gitlab/ModuleWithInstanceVariables }) end end @@ -29,7 +29,7 @@ module MilestoneActions format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_labels_tab", { - labels: @milestone.labels # rubocop:disable Cop/ModuleWithInstanceVariables + labels: @milestone.labels # rubocop:disable Gitlab/ModuleWithInstanceVariables }) end @@ -44,7 +44,7 @@ module MilestoneActions } end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def milestone_redirect_path if @project project_milestone_path(@project, @milestone) @@ -54,5 +54,5 @@ module MilestoneActions dashboard_milestone_path(@milestone.safe_title, title: @milestone.title) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index e6ef1f6f5e4..e82a5650935 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -31,7 +31,7 @@ module NotesActions render json: notes_json end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def create create_params = note_params.merge( merge_request_diff_head_sha: params[:merge_request_diff_head_sha], @@ -49,9 +49,9 @@ module NotesActions format.html { redirect_back_or_default } end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def update @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) @@ -64,7 +64,7 @@ module NotesActions format.html { redirect_back_or_default } end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def destroy if note.editable? @@ -143,7 +143,7 @@ module NotesActions end else template = "discussions/_diff_discussion" - @fresh_discussion = true # rubocop:disable Cop/ModuleWithInstanceVariables + @fresh_discussion = true # rubocop:disable Gitlab/ModuleWithInstanceVariables locals = { discussions: [discussion], on_image: on_image } end @@ -196,7 +196,7 @@ module NotesActions end def noteable - @noteable ||= notes_finder.target || @note&.noteable # rubocop:disable Cop/ModuleWithInstanceVariables + @noteable ||= notes_finder.target || @note&.noteable # rubocop:disable Gitlab/ModuleWithInstanceVariables end def require_noteable! diff --git a/app/controllers/concerns/preview_markdown.rb b/app/controllers/concerns/preview_markdown.rb index 01c95612922..738de424dac 100644 --- a/app/controllers/concerns/preview_markdown.rb +++ b/app/controllers/concerns/preview_markdown.rb @@ -1,7 +1,7 @@ module PreviewMarkdown extend ActiveSupport::Concern - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def preview_markdown result = PreviewMarkdownService.new(@project, current_user, params).execute @@ -20,5 +20,5 @@ module PreviewMarkdown } } end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb index 7b8cee435ee..fb41dc1e8a8 100644 --- a/app/controllers/concerns/renders_commits.rb +++ b/app/controllers/concerns/renders_commits.rb @@ -1,6 +1,6 @@ module RendersCommits def prepare_commits_for_rendering(commits) - Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Cop/ModuleWithInstanceVariables + Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables commits end diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb index 4ca6d9db5cf..e7ef297879f 100644 --- a/app/controllers/concerns/renders_notes.rb +++ b/app/controllers/concerns/renders_notes.rb @@ -1,5 +1,5 @@ module RendersNotes - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def prepare_notes_for_rendering(notes, noteable = nil) preload_noteable_for_regular_notes(notes) preload_max_access_for_authors(notes, @project) @@ -8,7 +8,7 @@ module RendersNotes notes end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables private diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index 47b10c3ef3b..3d61458c064 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -66,7 +66,7 @@ module ServiceParams FILTER_BLANK_PARAMS = [:password].freeze def service_params - dynamic_params = @service.event_channel_names + @service.event_names # rubocop:disable Cop/ModuleWithInstanceVariables + dynamic_params = @service.event_channel_names + @service.event_names # rubocop:disable Gitlab/ModuleWithInstanceVariables service_params = params.permit(:id, service: ALLOWED_PARAMS_CE + dynamic_params) if service_params[:service].is_a?(Hash) diff --git a/app/controllers/concerns/snippets_actions.rb b/app/controllers/concerns/snippets_actions.rb index 046dae480c1..9095cc7f783 100644 --- a/app/controllers/concerns/snippets_actions.rb +++ b/app/controllers/concerns/snippets_actions.rb @@ -4,7 +4,7 @@ module SnippetsActions def edit end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def raw disposition = params[:inline] == 'false' ? 'attachment' : 'inline' @@ -15,7 +15,7 @@ module SnippetsActions filename: @snippet.sanitized_file_name ) end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables private diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index fdea8bcb918..86f28f30032 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -46,7 +46,7 @@ module Noteable notes.inc_relations_for_view.grouped_diff_discussions(*args) end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def resolvable_discussions @resolvable_discussions ||= if defined?(@discussions) @@ -55,7 +55,7 @@ module Noteable discussion_notes.resolvable.discussions(self) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def discussions_resolvable? resolvable_discussions.any?(&:resolvable?) diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index 2ecce745ab9..835f26aa57b 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -52,7 +52,7 @@ module RelativePositioning # to its predecessor. This process will recursively move all the predecessors until we have a place if (after.relative_position - before.relative_position) < 2 before.move_before - @positionable_neighbours = [before] # rubocop:disable Cop/ModuleWithInstanceVariables + @positionable_neighbours = [before] # rubocop:disable Gitlab/ModuleWithInstanceVariables end self.relative_position = position_between(before.relative_position, after.relative_position) @@ -65,7 +65,7 @@ module RelativePositioning if before.shift_after? issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_after) issue_to_move.move_after - @positionable_neighbours = [issue_to_move] # rubocop:disable Cop/ModuleWithInstanceVariables + @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables pos_after = issue_to_move.relative_position end @@ -80,7 +80,7 @@ module RelativePositioning if after.shift_before? issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_before) issue_to_move.move_before - @positionable_neighbours = [issue_to_move] # rubocop:disable Cop/ModuleWithInstanceVariables + @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables pos_before = issue_to_move.relative_position end @@ -132,7 +132,7 @@ module RelativePositioning end end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def save_positionable_neighbours return unless @positionable_neighbours @@ -141,5 +141,5 @@ module RelativePositioning status end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index fc54a8bbca0..b6c7b6735b9 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -45,13 +45,13 @@ module ResolvableDiscussion def first_note_to_resolve return unless resolvable? - @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Cop/ModuleWithInstanceVariables + @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables end def last_resolved_note return unless resolved? - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Cop/ModuleWithInstanceVariables + @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables end def resolved_notes @@ -91,7 +91,7 @@ module ResolvableDiscussion yield(notes_relation) # Set the notes array to the updated notes - @notes = notes_relation.fresh.to_a # rubocop:disable Cop/ModuleWithInstanceVariables + @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables self.class.memoized_values.each do |var| instance_variable_set(:"@#{var}", nil) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index efec55d7376..5c1cce98ad4 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -88,7 +88,7 @@ module Routable def full_name if route && route.name.present? - @full_name ||= route.name # rubocop:disable Cop/ModuleWithInstanceVariables + @full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables else update_route if persisted? @@ -112,7 +112,7 @@ module Routable def expires_full_path_cache RequestStore.delete(full_path_key) if RequestStore.active? - @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables + @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables end def build_full_path @@ -127,7 +127,7 @@ module Routable def uncached_full_path if route && route.path.present? - @full_path ||= route.path # rubocop:disable Cop/ModuleWithInstanceVariables + @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables else update_route if persisted? @@ -166,7 +166,7 @@ module Routable route || build_route(source: self) route.path = build_full_path route.name = build_full_name - @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables - @full_name = nil # rubocop:disable Cop/ModuleWithInstanceVariables + @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables + @full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index 378a3ede2aa..d07041c2fdf 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -39,7 +39,7 @@ module Taskable def task_list_items return [] if description.blank? - @task_list_items ||= Taskable.get_tasks(description) # rubocop:disable Cop/ModuleWithInstanceVariables + @task_list_items ||= Taskable.get_tasks(description) # rubocop:disable Gitlab/ModuleWithInstanceVariables end def tasks diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index f1b43cb38e1..fc44a7da178 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -20,7 +20,7 @@ module TimeTrackable has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def spend_time(options) @time_spent = options[:duration] @time_spent_user = options[:user] @@ -36,7 +36,7 @@ module TimeTrackable end end alias_method :spend_time=, :spend_time - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def total_time_spent timelogs.sum(:time_spent) @@ -53,10 +53,10 @@ module TimeTrackable private def reset_spent_time - timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) # rubocop:disable Cop/ModuleWithInstanceVariables + timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def add_or_subtract_spent_time timelogs.new( time_spent: time_spent, @@ -64,7 +64,7 @@ module TimeTrackable spent_at: @spent_at ) end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/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 fc312aad8bd..26eb274f4d5 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -4,12 +4,12 @@ module Issues attr_reader :merge_request_to_resolve_discussions_of_iid, :discussion_to_resolve_id - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/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:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def merge_request_to_resolve_discussions_of strong_memoize(:merge_request_to_resolve_discussions_of) do @@ -22,7 +22,7 @@ module Issues def discussions_to_resolve return [] unless merge_request_to_resolve_discussions_of - @discussions_to_resolve ||= # rubocop:disable Cop/ModuleWithInstanceVariables + @discussions_to_resolve ||= # rubocop:disable Gitlab/ModuleWithInstanceVariables if discussion_to_resolve_id discussion_or_nil = merge_request_to_resolve_discussions_of .find_discussion(discussion_to_resolve_id) diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 31ec1e9713e..d4ade869777 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -7,19 +7,19 @@ # - params with :request # module SpamCheckService - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables 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 - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables # 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 + # rubocop:disable Gitlab/ModuleWithInstanceVariables def spam_check(spammable, user) spam_service = SpamService.new(spammable, @request) @@ -27,5 +27,5 @@ module SpamCheckService user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/workers/concerns/new_issuable.rb b/app/workers/concerns/new_issuable.rb index 9a15e79d0c3..526ed0bad07 100644 --- a/app/workers/concerns/new_issuable.rb +++ b/app/workers/concerns/new_issuable.rb @@ -9,15 +9,15 @@ module NewIssuable end def set_user(user_id) - @user = User.find_by(id: user_id) # rubocop:disable Cop/ModuleWithInstanceVariables + @user = User.find_by(id: user_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables - log_error(User, user_id) unless @user # rubocop:disable Cop/ModuleWithInstanceVariables + log_error(User, user_id) unless @user # rubocop:disable Gitlab/ModuleWithInstanceVariables end def set_issuable(issuable_id) - @issuable = issuable_class.find_by(id: issuable_id) # rubocop:disable Cop/ModuleWithInstanceVariables + @issuable = issuable_class.find_by(id: issuable_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables - log_error(issuable_class, issuable_id) unless @issuable # rubocop:disable Cop/ModuleWithInstanceVariables + log_error(issuable_class, issuable_id) unless @issuable # rubocop:disable Gitlab/ModuleWithInstanceVariables end def log_error(record_class, record_id) diff --git a/config/initializers/fix_local_cache_middleware.rb b/config/initializers/fix_local_cache_middleware.rb index 1f043408b4e..2644ee6a7d3 100644 --- a/config/initializers/fix_local_cache_middleware.rb +++ b/config/initializers/fix_local_cache_middleware.rb @@ -6,7 +6,7 @@ module LocalCacheRegistryCleanupWithEnsure def call(env) LocalCacheRegistry.set_cache_for(local_cache_key, LocalStore.new) - response = @app.call(env) # rubocop:disable Cop/ModuleWithInstanceVariables + response = @app.call(env) # rubocop:disable Gitlab/ModuleWithInstanceVariables response[2] = ::Rack::BodyProxy.new(response[2]) do LocalCacheRegistry.set_cache_for(local_cache_key, nil) end diff --git a/config/initializers/rspec_profiling.rb b/config/initializers/rspec_profiling.rb index c732e303f03..2de310753a9 100644 --- a/config/initializers/rspec_profiling.rb +++ b/config/initializers/rspec_profiling.rb @@ -19,10 +19,10 @@ module RspecProfilingExt def example_finished(*args) super rescue => err - return if @already_logged_example_finished_error # rubocop:disable Cop/ModuleWithInstanceVariables + return if @already_logged_example_finished_error # rubocop:disable Gitlab/ModuleWithInstanceVariables $stderr.puts "rspec_profiling couldn't collect an example: #{err}. Further warnings suppressed." - @already_logged_example_finished_error = true # rubocop:disable Cop/ModuleWithInstanceVariables + @already_logged_example_finished_error = true # rubocop:disable Gitlab/ModuleWithInstanceVariables end alias_method :example_passed, :example_finished diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 1c64b35b782..46e7177e704 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -185,7 +185,7 @@ Put the disabling comment right after your code in the same line: ``` ruby module M def violating_method - @f + @g # rubocop:disable Cop/ModuleWithInstanceVariables + @f + @g # rubocop:disable Gitlab/ModuleWithInstanceVariables end end ``` @@ -194,13 +194,13 @@ If there are multiple lines, you could also enable and disable for a section: ``` ruby module M - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def violating_method @f = 0 @g = 1 @h = 2 end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end ``` diff --git a/features/support/env.rb b/features/support/env.rb index a1413522f3a..b8df707a8ee 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? # rubocop:disable Cop/ModuleWithInstanceVariables + @max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? # rubocop:disable Gitlab/ModuleWithInstanceVariables name = scenario.name # This number has no significance, it's just to line things up - max_length = @max_step_name_length + 19 # rubocop:disable Cop/ModuleWithInstanceVariables + max_length = @max_step_name_length + 19 # rubocop:disable Gitlab/ModuleWithInstanceVariables 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 c4f81443282..fc1a3948bb4 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -32,7 +32,7 @@ module API end end - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables # We can't rewrite this with StrongMemoize because `sudo!` would # actually write to `@current_user`, and `sudo?` would immediately # call `current_user` again which reads from `@current_user`. @@ -50,7 +50,7 @@ module API @current_user end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def sudo? initial_current_user != current_user @@ -399,7 +399,7 @@ module API private - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def initial_current_user return @initial_current_user if defined?(@initial_current_user) @@ -409,7 +409,7 @@ module API unauthorized! end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def sudo! return unless sudo_identifier @@ -429,7 +429,7 @@ module API sudoed_user = find_user(sudo_identifier) not_found!("User with ID or username '#{sudo_identifier}'") unless sudoed_user - @current_user = sudoed_user # rubocop:disable Cop/ModuleWithInstanceVariables + @current_user = sudoed_user # rubocop:disable Gitlab/ModuleWithInstanceVariables end def sudo_identifier diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index e2077d33b9f..520bf65c3b3 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -9,13 +9,13 @@ module API attr_reader :redirected_path def wiki? - set_project unless defined?(@wiki) # rubocop:disable Cop/ModuleWithInstanceVariables - @wiki # rubocop:disable Cop/ModuleWithInstanceVariables + set_project unless defined?(@wiki) # rubocop:disable Gitlab/ModuleWithInstanceVariables + @wiki # rubocop:disable Gitlab/ModuleWithInstanceVariables end def project - set_project unless defined?(@project) # rubocop:disable Cop/ModuleWithInstanceVariables - @project # rubocop:disable Cop/ModuleWithInstanceVariables + set_project unless defined?(@project) # rubocop:disable Gitlab/ModuleWithInstanceVariables + @project # rubocop:disable Gitlab/ModuleWithInstanceVariables end def ssh_authentication_abilities @@ -67,7 +67,7 @@ module API private - # rubocop:disable Cop/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables def set_project if params[:gl_repository] @project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository]) @@ -76,7 +76,7 @@ module API @project, @wiki, @redirected_path = Gitlab::RepoPath.parse(params[:project]) end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables # Project id to pass between components that don't share/don't have # access to the same filesystem mounts diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index 40a65aad631..26f699f4c9d 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -40,7 +40,7 @@ module ExtractsPath def extract_ref(id) pair = ['', ''] - return pair unless @project # rubocop:disable Cop/ModuleWithInstanceVariables + return pair unless @project # rubocop:disable Gitlab/ModuleWithInstanceVariables if id =~ /^(\h{40})(.+)/ # If the ref appears to be a SHA, we're done, just split the string @@ -104,7 +104,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 + # rubocop:disable Gitlab/ModuleWithInstanceVariables def assign_ref_vars # assign allowed options allowed_options = ["filter_ref"] @@ -132,10 +132,10 @@ module ExtractsPath rescue RuntimeError, NoMethodError, InvalidPathError render_404 end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables def tree - @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Cop/ModuleWithInstanceVariables + @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables end private @@ -148,8 +148,8 @@ module ExtractsPath end def ref_names - return [] unless @project # rubocop:disable Cop/ModuleWithInstanceVariables + return [] unless @project # rubocop:disable Gitlab/ModuleWithInstanceVariables - @ref_names ||= @project.repository.ref_names # rubocop:disable Cop/ModuleWithInstanceVariables + @ref_names ||= @project.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/lib/gitlab/ci/charts.rb b/lib/gitlab/ci/charts.rb index e94166aee0c..525563a97f5 100644 --- a/lib/gitlab/ci/charts.rb +++ b/lib/gitlab/ci/charts.rb @@ -6,7 +6,7 @@ module Gitlab query .group("DATE(#{::Ci::Pipeline.table_name}.created_at)") .count(:created_at) - .transform_keys { |date| date.strftime(@format) } # rubocop:disable Cop/ModuleWithInstanceVariables + .transform_keys { |date| date.strftime(@format) } # rubocop:disable Gitlab/ModuleWithInstanceVariables end def interval_step diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index 0dc359a86c3..e45787773a8 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -13,7 +13,7 @@ module Gitlab end def errors - @validator.messages + descendants.flat_map(&:errors) # rubocop:disable Cop/ModuleWithInstanceVariables + @validator.messages + descendants.flat_map(&:errors) # rubocop:disable Gitlab/ModuleWithInstanceVariables end class_methods do diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index dfb2cfe42b7..91fd9cc7631 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -53,7 +53,7 @@ module Gitlab end def in_memory_application_settings - @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) # rubocop:disable Cop/ModuleWithInstanceVariables + @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) # rubocop:disable Gitlab/ModuleWithInstanceVariables rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError # In case migrations the application_settings table is not created yet, # we fallback to a simple OpenStruct diff --git a/lib/gitlab/cycle_analytics/base_query.rb b/lib/gitlab/cycle_analytics/base_query.rb index 05b4928c3b9..dcbdf9a64b0 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -14,9 +14,9 @@ module Gitlab 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])) - .where(issue_table[:project_id].eq(@project.id)) # rubocop:disable Cop/ModuleWithInstanceVariables + .where(issue_table[:project_id].eq(@project.id)) # rubocop:disable Gitlab/ModuleWithInstanceVariables .where(issue_table[:deleted_at].eq(nil)) - .where(issue_table[:created_at].gteq(@options[:from])) # rubocop:disable Cop/ModuleWithInstanceVariables + .where(issue_table[:created_at].gteq(@options[:from])) # rubocop:disable Gitlab/ModuleWithInstanceVariables # Load merge_requests query = query.join(mr_table, Arel::Nodes::OuterJoin) diff --git a/lib/gitlab/cycle_analytics/production_helper.rb b/lib/gitlab/cycle_analytics/production_helper.rb index ebbff1b2f33..7a889b3877f 100644 --- a/lib/gitlab/cycle_analytics/production_helper.rb +++ b/lib/gitlab/cycle_analytics/production_helper.rb @@ -4,7 +4,7 @@ module Gitlab def stage_query super .where(mr_metrics_table[:first_deployed_to_production_at] - .gteq(@options[:from])) # rubocop:disable Cop/ModuleWithInstanceVariables + .gteq(@options[:from])) # rubocop:disable Gitlab/ModuleWithInstanceVariables 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 9ae1f66a182..403afbe3b9a 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 @@ -6,7 +6,7 @@ module Gitlab module Routable def full_path if route && route.path.present? - @full_path ||= route.path # rubocop:disable Cop/ModuleWithInstanceVariables + @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables else update_route if persisted? @@ -30,7 +30,7 @@ module Gitlab def prepare_route route || build_route(source: self) route.path = build_full_path - @full_path = nil # rubocop:disable Cop/ModuleWithInstanceVariables + @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 19dab0037bb..0135b3c6f22 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -32,7 +32,7 @@ module Gitlab def execute(cmd) output, status = Gitlab::Popen.popen(cmd) - @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? # rubocop:disable Cop/ModuleWithInstanceVariables + @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? # rubocop:disable Gitlab/ModuleWithInstanceVariables status.zero? end diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index fa88d41be73..6ea132fc5bf 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -154,7 +154,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 + # rubocop:disable Gitlab/ModuleWithInstanceVariables def pool if influx_metrics_enabled? if @pool.nil? @@ -171,7 +171,7 @@ module Gitlab @pool end end - # rubocop:enable Cop/ModuleWithInstanceVariables + # rubocop:enable Gitlab/ModuleWithInstanceVariables end end end diff --git a/rubocop/cop/gitlab/module_with_instance_variables.rb b/rubocop/cop/gitlab/module_with_instance_variables.rb new file mode 100644 index 00000000000..5c9cde98512 --- /dev/null +++ b/rubocop/cop/gitlab/module_with_instance_variables.rb @@ -0,0 +1,63 @@ +module RuboCop + module Cop + module Gitlab + class ModuleWithInstanceVariables < RuboCop::Cop::Cop + MSG = <<~EOL.freeze + Do not use instance variables in a module. Please read this + for the rationale behind it: + + https://docs.gitlab.com/ee/development/module_with_instance_variables.html + EOL + + def on_module(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 + + 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 + # We allow initialize method and single ivar + elsif !initialize_method?(definition) && !single_ivar?(definition) + 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 + + def single_ivar?(definition) + node = definition.child_nodes.last + + definition.child_nodes.size == 2 && node.ivar_type? + end + + def initialize_method?(definition) + definition.children.first == :initialize + end + end + end + end +end diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb deleted file mode 100644 index f101ae09ad2..00000000000 --- a/rubocop/cop/module_with_instance_variables.rb +++ /dev/null @@ -1,82 +0,0 @@ -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: - - https://docs.gitlab.com/ee/development/module_with_instance_variables.html - 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 - # We allow initialize method and single ivar - elsif !initialize_method?(definition) && !single_ivar?(definition) - 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 - - def single_ivar?(definition) - node = definition.child_nodes.last - - definition.child_nodes.size == 2 && node.ivar_type? - end - - def initialize_method?(definition) - definition.children.first == :initialize - end - end - end -end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 65a37f578cd..c4bab18faee 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -5,8 +5,8 @@ require_relative 'cop/gem_fetcher' require_relative 'cop/in_batches' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' -require_relative 'cop/module_with_instance_variables' require_relative 'cop/redirect_with_status' +require_relative 'cop/gitlab/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/gitlab/module_with_instance_variables_spec.rb similarity index 77% rename from spec/rubocop/cop/module_with_instance_variables_spec.rb rename to spec/rubocop/cop/gitlab/module_with_instance_variables_spec.rb index df5e2dd2f04..1fd40653f79 100644 --- a/spec/rubocop/cop/module_with_instance_variables_spec.rb +++ b/spec/rubocop/cop/gitlab/module_with_instance_variables_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' require 'rubocop' require 'rubocop/rspec/support' -require_relative '../../../rubocop/cop/module_with_instance_variables' +require_relative '../../../../rubocop/cop/gitlab/module_with_instance_variables' -describe RuboCop::Cop::ModuleWithInstanceVariables do +describe RuboCop::Cop::Gitlab::ModuleWithInstanceVariables do include CopHelper subject(:cop) { described_class.new } @@ -83,42 +83,6 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do end 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 it_behaves_like 'not registering offense' do let(:source) do From 89f29395259326793435044261641f5571e718d0 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 16:10:19 +0800 Subject: [PATCH 12/16] Reword Instance variables in views --- doc/development/module_with_instance_variables.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 46e7177e704..ff8d4dacd11 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -216,13 +216,9 @@ other random objects, so they're still somewhat isolated. ### Instance variables in views -They're terrible, because they're also shared between different controllers, -and it's very hard to track where those instance variables were set when we -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. +They're bad because we can't easily tell who's using the instance variables +(from controller's point of view) and where we set them up (from partials' +point of view), making it extremely hard to track data dependency. We're trying to use something like this instead: @@ -238,5 +234,6 @@ And in the partial: - project = local_assigns.fetch(:project) ``` -This way it's clearer where those values were coming from. In the future, +This way it's clearer where those values were coming from, and we gain the +benefit to have typo check over using instance variables. In the future, we should also forbid the use of instance variables in partials. From 166a2d7a67787d3cf8cebb1e75fc557e2409e669 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 16:34:52 +0800 Subject: [PATCH 13/16] Make it clear that this is an acceptable use --- doc/development/module_with_instance_variables.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index ff8d4dacd11..48a1b7f847e 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -77,8 +77,7 @@ 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: +could be easily rewritten in simple form. Consider this acceptable method: ``` ruby module Gitlab @@ -92,8 +91,12 @@ module Gitlab end ``` -It's still offending because it's not just `||=`, but we could split this -method into two: +This method is totally fine because it's already self-contained. No other +methods should be using `@emoji_unicode_versions_by_name` and we're good. +However it's still offending the cop because it's not just `||=`, and the +cop is not smart enough to judge that this is fine. + +On the other hand, we could split this method into two: ``` ruby module Gitlab @@ -112,7 +115,7 @@ module Gitlab end ``` -Now the cop won't complain. Here's another bad example which we could rewrite: +Now the cop won't complain. Here's a bad example which we could rewrite: ``` ruby module SpamCheckService From 418947b6ce3da1c62b6449c4782d3e979eb82a07 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 17:15:46 +0800 Subject: [PATCH 14/16] Fix a few layout error --- app/controllers/concerns/issuable_actions.rb | 2 +- app/controllers/concerns/milestone_actions.rb | 1 - app/models/concerns/time_trackable.rb | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index a725aad43e7..6c503bc8e55 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -98,7 +98,7 @@ module IssuableActions end def labels - @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables + @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables end def authorize_destroy_issuable! diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index 7bec64c5d60..d92cf8b4894 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -30,7 +30,6 @@ module MilestoneActions format.json do render json: tabs_json("shared/milestones/_labels_tab", { labels: @milestone.labels # rubocop:disable Gitlab/ModuleWithInstanceVariables - }) end end diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index fc44a7da178..89fe6527647 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -4,6 +4,7 @@ # # Used by Issue and MergeRequest. # + module TimeTrackable extend ActiveSupport::Concern From 689658456f706be7278fbf50fcde9c7f43cd0655 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 22 Nov 2017 17:32:10 +0800 Subject: [PATCH 15/16] Cache allowed_ids --- lib/gitlab/cycle_analytics/base_event_fetcher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/cycle_analytics/base_event_fetcher.rb b/lib/gitlab/cycle_analytics/base_event_fetcher.rb index 62fc3ee3b5f..e3e3767cc75 100644 --- a/lib/gitlab/cycle_analytics/base_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/base_event_fetcher.rb @@ -56,7 +56,7 @@ module Gitlab end def allowed_ids - allowed_ids_finder_class + @allowed_ids ||= allowed_ids_finder_class .new(@options[:current_user], project_id: @project.id) .execute.where(id: event_result_ids).pluck(:id) end From 02994fbe7715ef4539f720b6d395eeb9a3d71f14 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 15 Dec 2017 19:37:57 +0800 Subject: [PATCH 16/16] Backport changes from EE --- .rubocop.yml | 4 ++-- app/controllers/concerns/issuable_actions.rb | 2 +- lib/extracts_path.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index d103a14518f..7721cfaf850 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1191,9 +1191,9 @@ Gitlab/ModuleWithInstanceVariables: Enable: true Exclude: # We ignore Rails helpers right now because it's hard to workaround it - - app/helpers/*_helper.rb + - app/helpers/**/*_helper.rb # We ignore Rails mailers right now because it's hard to workaround it - - app/mailers/emails/*.rb + - app/mailers/emails/**/*.rb # We ignore spec helpers because it usually doesn't matter - spec/support/**/*.rb - features/steps/**/*.rb diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index a2b6ec7bb6a..c3013884369 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -81,7 +81,7 @@ module IssuableActions private def recaptcha_check_if_spammable(should_redirect = true, &block) - return yield unless @issuable.is_a? Spammable + return yield unless issuable.is_a? Spammable recaptcha_check_with_fallback(should_redirect, &block) end diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index aa9996c7685..d8aca3304c5 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -139,7 +139,7 @@ module ExtractsPath def lfs_blob_ids blob_ids = tree.blobs.map(&:id) - @lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(@project.repository, blob_ids).map(&:id) + @lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(@project.repository, blob_ids).map(&:id) # rubocop:disable Gitlab/ModuleWithInstanceVariables end private