diff options
53 files changed, 220 insertions, 109 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 +module M + def f + @f ||= true + end +end +``` + +Unfortunately it's not easy to code more complex rules into the cop, so +we rely on people's best 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 - prepend UseGitlabGitAttributes + 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 ``` -Here's a bad example which we should rewrite: +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) + 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 - it 'does not register offenses' do - inspect_source(cop, <<~RUBY) + 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 - - expect(cop.offenses).to be_empty 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) + 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 - it 'does not register offenses' do - inspect_source(cop, <<~RUBY) + 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 ||= true + @f.to_s end end RUBY - - expect(cop.offenses).to be_empty end + + let(:offending_lines) { [3, 4] } + + it_behaves_like 'registering offense' end end |