BigW Consortium Gitlab

Commit 6a4ee9aa by Lin Jen-Shin

Allow simple ivar ||= form. Update accordingly

parent 9ae92b8c
# rubocop:disable Cop/ModuleWithInstanceVariables
module BoardsResponses module BoardsResponses
def authorize_read_list def authorize_read_list
authorize_action_for!(board.parent, :read_list) authorize_action_for!(board.parent, :read_list)
...@@ -24,10 +23,12 @@ module BoardsResponses ...@@ -24,10 +23,12 @@ module BoardsResponses
return render_403 unless can?(current_user, ability, resource) return render_403 unless can?(current_user, ability, resource)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def respond_with_boards def respond_with_boards
respond_with(@boards) respond_with(@boards)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def respond_with_board def respond_with_board
respond_with(@board) respond_with(@board)
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module CreatesCommit module CreatesCommit
extend ActiveSupport::Concern extend ActiveSupport::Concern
# rubocop:disable Cop/ModuleWithInstanceVariables
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
if can?(current_user, :push_code, @project) if can?(current_user, :push_code, @project)
@project_to_commit_into = @project @project_to_commit_into = @project
...@@ -78,6 +78,7 @@ module CreatesCommit ...@@ -78,6 +78,7 @@ module CreatesCommit
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def new_merge_request_path def new_merge_request_path
project_new_merge_request_path( project_new_merge_request_path(
@project_to_commit_into, @project_to_commit_into,
...@@ -94,6 +95,7 @@ module CreatesCommit ...@@ -94,6 +95,7 @@ module CreatesCommit
project_merge_request_path(@project, @merge_request) project_merge_request_path(@project, @merge_request)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def merge_request_exists? def merge_request_exists?
return @merge_request if defined?(@merge_request) return @merge_request if defined?(@merge_request)
...@@ -101,10 +103,12 @@ module CreatesCommit ...@@ -101,10 +103,12 @@ module CreatesCommit
.find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch) .find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def different_project? def different_project?
@project_to_commit_into != @project @project_to_commit_into != @project
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def create_merge_request? def create_merge_request?
# Even if the field is set, if we're checking the same branch # Even if the field is set, if we're checking the same branch
# as the target branch in the same project, # as the target branch in the same project,
......
module CycleAnalyticsParams module CycleAnalyticsParams
extend ActiveSupport::Concern extend ActiveSupport::Concern
# rubocop:disable Cop/ModuleWithInstanceVariables
def options(params) def options(params)
@options ||= { from: start_date(params), current_user: current_user } @options ||= { from: start_date(params), current_user: current_user }
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module IssuableActions module IssuableActions
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -8,6 +7,7 @@ module IssuableActions ...@@ -8,6 +7,7 @@ module IssuableActions
before_action :authorize_admin_issuable!, only: :bulk_update before_action :authorize_admin_issuable!, only: :bulk_update
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def destroy def destroy
issuable.destroy issuable.destroy
destroy_method = "destroy_#{issuable.class.name.underscore}".to_sym destroy_method = "destroy_#{issuable.class.name.underscore}".to_sym
...@@ -36,6 +36,7 @@ module IssuableActions ...@@ -36,6 +36,7 @@ module IssuableActions
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def render_conflict_response def render_conflict_response
respond_to do |format| respond_to do |format|
format.html do format.html do
...@@ -53,6 +54,7 @@ module IssuableActions ...@@ -53,6 +54,7 @@ module IssuableActions
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def labels def labels
@labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
end end
...@@ -63,6 +65,7 @@ module IssuableActions ...@@ -63,6 +65,7 @@ module IssuableActions
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def authorize_admin_issuable! def authorize_admin_issuable!
unless can?(current_user, :"admin_#{resource_name}", @project) unless can?(current_user, :"admin_#{resource_name}", @project)
return access_denied! return access_denied!
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module IssuableCollections module IssuableCollections
extend ActiveSupport::Concern extend ActiveSupport::Concern
include SortingHelper include SortingHelper
...@@ -11,6 +10,7 @@ module IssuableCollections ...@@ -11,6 +10,7 @@ module IssuableCollections
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def set_issues_index def set_issues_index
@collection_type = "Issue" @collection_type = "Issue"
@issues = issues_collection @issues = issues_collection
...@@ -85,6 +85,7 @@ module IssuableCollections ...@@ -85,6 +85,7 @@ module IssuableCollections
finder_class.new(current_user, filter_params) finder_class.new(current_user, filter_params)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def filter_params def filter_params
set_sort_order_from_cookie set_sort_order_from_cookie
set_default_state set_default_state
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module IssuesAction module IssuesAction
extend ActiveSupport::Concern extend ActiveSupport::Concern
include IssuableCollections include IssuableCollections
# rubocop:disable Cop/ModuleWithInstanceVariables
def issues def issues
@label = issues_finder.labels.first @label = issues_finder.labels.first
......
...@@ -90,7 +90,6 @@ module LfsRequest ...@@ -90,7 +90,6 @@ module LfsRequest
has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project) has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def storage_project def storage_project
@storage_project ||= begin @storage_project ||= begin
result = project result = project
...@@ -104,7 +103,6 @@ module LfsRequest ...@@ -104,7 +103,6 @@ module LfsRequest
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def objects def objects
@objects ||= (params[:objects] || []).to_a @objects ||= (params[:objects] || []).to_a
end end
......
...@@ -76,7 +76,6 @@ module MembershipActions ...@@ -76,7 +76,6 @@ module MembershipActions
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def source_type def source_type
@source_type ||= membershipable.class.to_s.humanize(capitalize: false) @source_type ||= membershipable.class.to_s.humanize(capitalize: false)
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module MergeRequestsAction module MergeRequestsAction
extend ActiveSupport::Concern extend ActiveSupport::Concern
include IssuableCollections include IssuableCollections
# rubocop:disable Cop/ModuleWithInstanceVariables
def merge_requests def merge_requests
@label = merge_requests_finder.labels.first @label = merge_requests_finder.labels.first
......
...@@ -13,8 +13,7 @@ module OauthApplications ...@@ -13,8 +13,7 @@ module OauthApplications
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def load_scopes def load_scopes
@scopes = Doorkeeper.configuration.scopes @scopes ||= Doorkeeper.configuration.scopes
end end
end end
...@@ -17,7 +17,6 @@ module RequiresWhitelistedMonitoringClient ...@@ -17,7 +17,6 @@ module RequiresWhitelistedMonitoringClient
ip_whitelist.any? { |e| e.include?(Gitlab::RequestContext.client_ip) } ip_whitelist.any? { |e| e.include?(Gitlab::RequestContext.client_ip) }
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def ip_whitelist def ip_whitelist
@ip_whitelist ||= Settings.monitoring.ip_whitelist.map(&IPAddr.method(:new)) @ip_whitelist ||= Settings.monitoring.ip_whitelist.map(&IPAddr.method(:new))
end end
......
...@@ -17,7 +17,6 @@ module IgnorableColumn ...@@ -17,7 +17,6 @@ module IgnorableColumn
super.reject { |column| ignored_columns.include?(column.name) } super.reject { |column| ignored_columns.include?(column.name) }
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def ignored_columns def ignored_columns
@ignored_columns ||= Set.new @ignored_columns ||= Set.new
end end
......
...@@ -5,7 +5,6 @@ ...@@ -5,7 +5,6 @@
# #
# Used by Issue, Note, MergeRequest, and Commit. # Used by Issue, Note, MergeRequest, and Commit.
# #
# # rubocop:disable Cop/ModuleWithInstanceVariables
module Mentionable module Mentionable
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -44,6 +43,7 @@ module Mentionable ...@@ -44,6 +43,7 @@ module Mentionable
self self
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def all_references(current_user = nil, extractor: nil) def all_references(current_user = nil, extractor: nil)
@extractors ||= {} @extractors ||= {}
......
...@@ -12,7 +12,6 @@ module Noteable ...@@ -12,7 +12,6 @@ module Noteable
# #
# noteable.class # => MergeRequest # noteable.class # => MergeRequest
# noteable.human_class_name # => "merge request" # noteable.human_class_name # => "merge request"
# rubocop:disable Cop/ModuleWithInstanceVariables
def human_class_name def human_class_name
@human_class_name ||= base_class_name.titleize.downcase @human_class_name ||= base_class_name.titleize.downcase
end end
...@@ -35,7 +34,6 @@ module Noteable ...@@ -35,7 +34,6 @@ module Noteable
delegate :find_discussion, to: :discussion_notes delegate :find_discussion, to: :discussion_notes
# rubocop:disable Cop/ModuleWithInstanceVariables
def discussions def discussions
@discussions ||= discussion_notes @discussions ||= discussion_notes
.inc_relations_for_view .inc_relations_for_view
...@@ -70,7 +68,6 @@ module Noteable ...@@ -70,7 +68,6 @@ module Noteable
discussions_resolvable? && !discussions_resolved? discussions_resolvable? && !discussions_resolved?
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def discussions_to_be_resolved def discussions_to_be_resolved
@discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?) @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?)
end end
......
...@@ -55,17 +55,18 @@ module Participable ...@@ -55,17 +55,18 @@ module Participable
# This method processes attributes of objects in breadth-first order. # This method processes attributes of objects in breadth-first order.
# #
# Returns an Array of User instances. # Returns an Array of User instances.
# rubocop:disable Cop/ModuleWithInstanceVariables
def participants(current_user = nil) def participants(current_user = nil)
@participants ||= Hash.new do |hash, user| all_participants[current_user]
hash[user] = raw_participants(user)
end
@participants[current_user]
end end
private private
def all_participants
@all_participants ||= Hash.new do |hash, user|
hash[user] = raw_participants(user)
end
end
def raw_participants(current_user = nil) def raw_participants(current_user = nil)
current_user ||= author current_user ||= author
ext = Gitlab::ReferenceExtractor.new(project, current_user) ext = Gitlab::ReferenceExtractor.new(project, current_user)
......
...@@ -55,7 +55,6 @@ module ProtectedRef ...@@ -55,7 +55,6 @@ module ProtectedRef
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def ref_matcher def ref_matcher
@ref_matcher ||= ProtectedRefMatcher.new(self) @ref_matcher ||= ProtectedRefMatcher.new(self)
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module RelativePositioning module RelativePositioning
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -45,6 +44,7 @@ module RelativePositioning ...@@ -45,6 +44,7 @@ module RelativePositioning
next_pos next_pos
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def move_between(before, after) def move_between(before, after)
return move_after(before) unless after return move_after(before) unless after
return move_before(after) unless before return move_before(after) unless before
...@@ -59,6 +59,7 @@ module RelativePositioning ...@@ -59,6 +59,7 @@ module RelativePositioning
self.relative_position = position_between(before.relative_position, after.relative_position) self.relative_position = position_between(before.relative_position, after.relative_position)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def move_after(before = self) def move_after(before = self)
pos_before = before.relative_position pos_before = before.relative_position
pos_after = before.next_relative_position pos_after = before.next_relative_position
...@@ -74,6 +75,7 @@ module RelativePositioning ...@@ -74,6 +75,7 @@ module RelativePositioning
self.relative_position = position_between(pos_before, pos_after) self.relative_position = position_between(pos_before, pos_after)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def move_before(after = self) def move_before(after = self)
pos_after = after.relative_position pos_after = after.relative_position
pos_before = after.prev_relative_position pos_before = after.prev_relative_position
...@@ -133,6 +135,7 @@ module RelativePositioning ...@@ -133,6 +135,7 @@ module RelativePositioning
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def save_positionable_neighbours def save_positionable_neighbours
return unless @positionable_neighbours return unless @positionable_neighbours
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module ResolvableDiscussion module ResolvableDiscussion
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -31,12 +30,14 @@ module ResolvableDiscussion ...@@ -31,12 +30,14 @@ module ResolvableDiscussion
allow_nil: true allow_nil: true
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def resolvable? def resolvable?
return @resolvable if @resolvable.present? return @resolvable if @resolvable.present?
@resolvable = potentially_resolvable? && notes.any?(&:resolvable?) @resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def resolved? def resolved?
return @resolved if @resolved.present? return @resolved if @resolved.present?
...@@ -47,12 +48,14 @@ module ResolvableDiscussion ...@@ -47,12 +48,14 @@ module ResolvableDiscussion
@first_note ||= notes.first @first_note ||= notes.first
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def first_note_to_resolve def first_note_to_resolve
return unless resolvable? return unless resolvable?
@first_note_to_resolve ||= notes.find(&:to_be_resolved?) @first_note_to_resolve ||= notes.find(&:to_be_resolved?)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def last_resolved_note def last_resolved_note
return unless resolved? return unless resolved?
...@@ -89,6 +92,7 @@ module ResolvableDiscussion ...@@ -89,6 +92,7 @@ module ResolvableDiscussion
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def update def update
# Do not select `Note.resolvable`, so that system notes remain in the collection # Do not select `Note.resolvable`, so that system notes remain in the collection
notes_relation = Note.where(id: notes.map(&:id)) notes_relation = Note.where(id: notes.map(&:id))
......
# Store object full path in separate table for easy lookup and uniq validation # 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. # Object must have name and path db fields and respond to parent and parent_changed? methods.
# rubocop:disable Cop/ModuleWithInstanceVariables
module Routable module Routable
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -87,6 +86,7 @@ module Routable ...@@ -87,6 +86,7 @@ module Routable
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def full_name def full_name
if route && route.name.present? if route && route.name.present?
@full_name ||= route.name @full_name ||= route.name
...@@ -107,6 +107,7 @@ module Routable ...@@ -107,6 +107,7 @@ module Routable
RequestStore[full_path_key] ||= uncached_full_path RequestStore[full_path_key] ||= uncached_full_path
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def expires_full_path_cache def expires_full_path_cache
RequestStore.delete(full_path_key) if RequestStore.active? RequestStore.delete(full_path_key) if RequestStore.active?
@full_path = nil @full_path = nil
...@@ -122,6 +123,7 @@ module Routable ...@@ -122,6 +123,7 @@ module Routable
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def uncached_full_path def uncached_full_path
if route && route.path.present? if route && route.path.present?
@full_path ||= route.path @full_path ||= route.path
...@@ -157,6 +159,7 @@ module Routable ...@@ -157,6 +159,7 @@ module Routable
route.save route.save
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def prepare_route def prepare_route
route || build_route(source: self) route || build_route(source: self)
route.path = build_full_path route.path = build_full_path
......
...@@ -49,7 +49,6 @@ module Storage ...@@ -49,7 +49,6 @@ module Storage
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def old_repository_storage_paths def old_repository_storage_paths
@old_repository_storage_paths ||= repository_storage_paths @old_repository_storage_paths ||= repository_storage_paths
end end
......
...@@ -17,7 +17,6 @@ module StripAttribute ...@@ -17,7 +17,6 @@ module StripAttribute
strip_attrs.concat(attrs) strip_attrs.concat(attrs)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def strip_attrs def strip_attrs
@strip_attrs ||= [] @strip_attrs ||= []
end end
......
...@@ -6,7 +6,6 @@ require 'task_list/filter' ...@@ -6,7 +6,6 @@ require 'task_list/filter'
# bugs". # bugs".
# #
# Used by MergeRequest and Issue # Used by MergeRequest and Issue
# rubocop:disable Cop/ModuleWithInstanceVariables
module Taskable module Taskable
COMPLETED = 'completed'.freeze COMPLETED = 'completed'.freeze
INCOMPLETE = 'incomplete'.freeze INCOMPLETE = 'incomplete'.freeze
...@@ -37,6 +36,7 @@ module Taskable ...@@ -37,6 +36,7 @@ module Taskable
end end
# Called by `TaskList::Summary` # Called by `TaskList::Summary`
# rubocop:disable Cop/ModuleWithInstanceVariables
def task_list_items def task_list_items
return [] if description.blank? return [] if description.blank?
......
...@@ -4,7 +4,6 @@ ...@@ -4,7 +4,6 @@
# #
# Used by Issue and MergeRequest. # Used by Issue and MergeRequest.
# #
# rubocop:disable Cop/ModuleWithInstanceVariables
module TimeTrackable module TimeTrackable
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -21,6 +20,7 @@ module TimeTrackable ...@@ -21,6 +20,7 @@ module TimeTrackable
has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def spend_time(options) def spend_time(options)
@time_spent = options[:duration] @time_spent = options[:duration]
@time_spent_user = options[:user] @time_spent_user = options[:user]
...@@ -50,6 +50,7 @@ module TimeTrackable ...@@ -50,6 +50,7 @@ module TimeTrackable
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def reset_spent_time 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)
end end
...@@ -58,6 +59,7 @@ module TimeTrackable ...@@ -58,6 +59,7 @@ module TimeTrackable
timelogs.new(time_spent: time_spent, user: @time_spent_user) timelogs.new(time_spent: time_spent, user: @time_spent_user)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def check_negative_time_spent def check_negative_time_spent
return if time_spent.nil? || time_spent == :reset return if time_spent.nil? || time_spent == :reset
......
...@@ -6,8 +6,8 @@ ...@@ -6,8 +6,8 @@
# Dependencies: # Dependencies:
# - params with :request # - params with :request
# #
# rubocop:disable Cop/ModuleWithInstanceVariables
module SpamCheckService module SpamCheckService
# rubocop:disable Cop/ModuleWithInstanceVariables
def filter_spam_check_params def filter_spam_check_params
@request = params.delete(:request) @request = params.delete(:request)
@api = params.delete(:api) @api = params.delete(:api)
...@@ -18,6 +18,7 @@ module SpamCheckService ...@@ -18,6 +18,7 @@ module SpamCheckService
# In order to be proceed to the spam check process, @spammable has to be # 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 # a dirty instance, which means it should be already assigned with the new
# attribute values. # attribute values.
# rubocop:disable Cop/ModuleWithInstanceVariables
def spam_check(spammable, user) def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request) spam_service = SpamService.new(spammable, @request)
......
...@@ -662,7 +662,6 @@ module SystemNoteService ...@@ -662,7 +662,6 @@ module SystemNoteService
Rack::Utils.escape_html(text) Rack::Utils.escape_html(text)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def url_helpers def url_helpers
@url_helpers ||= Gitlab::Routing.url_helpers @url_helpers ||= Gitlab::Routing.url_helpers
end end
......
...@@ -11,10 +11,10 @@ ...@@ -11,10 +11,10 @@
# anyway, and there is no great efficiency gain from just fetching the listed # anyway, and there is no great efficiency gain from just fetching the listed
# attributes with our implementation, so we ignore the additional arguments. # attributes with our implementation, so we ignore the additional arguments.
# #
# rubocop:disable Cop/ModuleWithInstanceVariables
module Rugged module Rugged
class Repository class Repository
module UseGitlabGitAttributes module UseGitlabGitAttributes
# rubocop:disable Cop/ModuleWithInstanceVariables
def fetch_attributes(name, *) def fetch_attributes(name, *)
@attributes ||= Gitlab::Git::Attributes.new(path) @attributes ||= Gitlab::Git::Attributes.new(path)
@attributes.attributes(name) @attributes.attributes(name)
......
## Usually modules with instance variables considered harmful ## Modules with instance variables could be considered harmful
### Background ### Background
...@@ -43,7 +43,7 @@ instance variables in the final giant object, and that's where the problem is. ...@@ -43,7 +43,7 @@ instance variables in the final giant object, and that's where the problem is.
### Solutions ### Solutions
We should split the giant object into multiple objects, and they communicate 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 inheritance. This way, each smaller objects would have their own respective
limited states, i.e. instance variables. If one instance variable goes wrong, 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 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 ...@@ -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 to debug and track, and much more extensible for other objects to use, because
they communicate in a clear way, rather than implicit dependencies. 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, 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 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 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 use.
for now we rely on people turning off the cops, if they think that the use
conform this rule.
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 ``` ruby
# This is ok, as long as `@attributes` is never used anywhere else. module M
# Consider adding some prefix or suffix to avoid name conflicts though. def f
# rubocop:disable Cop/ModuleWithInstanceVariables @f ||= true
module Rugged end
class Repository end
module UseGitlabGitAttributes ```
def fetch_attributes(name, *)
@attributes ||= Gitlab::Git::Attributes.new(path) Unfortunately it's not easy to code more complex rules into the cop, so
@attributes.attributes(name) we rely on people's best judge. If we could find another good pattern we
end 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 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
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 ``` ruby
module SpamCheckService module SpamCheckService
...@@ -146,7 +177,7 @@ end ...@@ -146,7 +177,7 @@ end
This way, all those instance variables are isolated in `SpamCheckService` This way, all those instance variables are isolated in `SpamCheckService`
rather than who ever include the module, and those modules which were also 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, 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 ### Things we might need to ignore right now
......
...@@ -20,7 +20,6 @@ module AfterCommitQueue ...@@ -20,7 +20,6 @@ module AfterCommitQueue
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def _after_commit_queue def _after_commit_queue
@after_commit_queue ||= [] @after_commit_queue ||= []
end end
......
...@@ -2,7 +2,6 @@ ...@@ -2,7 +2,6 @@
require 'rack/oauth2' require 'rack/oauth2'
# rubocop:disable Cop/ModuleWithInstanceVariables
module API module API
module APIGuard module APIGuard
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -43,6 +42,8 @@ module API ...@@ -43,6 +42,8 @@ module API
# Helper Methods for Grape Endpoint # Helper Methods for Grape Endpoint
module HelperMethods module HelperMethods
attr_reader :current_user
# Invokes the doorkeeper guard. # Invokes the doorkeeper guard.
# #
# If token is presented and valid, then it sets @current_user. # If token is presented and valid, then it sets @current_user.
...@@ -61,6 +62,7 @@ module API ...@@ -61,6 +62,7 @@ module API
# scopes: (optional) scopes required for this guard. # scopes: (optional) scopes required for this guard.
# Defaults to empty array. # Defaults to empty array.
# #
# rubocop:disable Cop/ModuleWithInstanceVariables
def doorkeeper_guard(scopes: []) def doorkeeper_guard(scopes: [])
access_token = find_access_token access_token = find_access_token
return nil unless access_token return nil unless access_token
...@@ -88,10 +90,6 @@ module API ...@@ -88,10 +90,6 @@ module API
find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes) find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes)
end end
def current_user
@current_user
end
private private
def find_user_by_authentication_token(token_string) def find_user_by_authentication_token(token_string)
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module API module API
module Helpers module Helpers
include Gitlab::Utils include Gitlab::Utils
...@@ -33,6 +32,7 @@ module API ...@@ -33,6 +32,7 @@ module API
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def current_user def current_user
return @current_user if defined?(@current_user) return @current_user if defined?(@current_user)
...@@ -396,6 +396,7 @@ module API ...@@ -396,6 +396,7 @@ module API
warden.try(:authenticate) if verified_request? warden.try(:authenticate) if verified_request?
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def initial_current_user def initial_current_user
return @initial_current_user if defined?(@initial_current_user) return @initial_current_user if defined?(@initial_current_user)
Gitlab::Auth::UniqueIpsLimiter.limit_user! do Gitlab::Auth::UniqueIpsLimiter.limit_user! do
...@@ -411,6 +412,7 @@ module API ...@@ -411,6 +412,7 @@ module API
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def sudo! def sudo!
return unless sudo_identifier return unless sudo_identifier
return unless initial_current_user return unless initial_current_user
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module API module API
module Helpers module Helpers
module InternalHelpers module InternalHelpers
...@@ -7,20 +6,20 @@ module API ...@@ -7,20 +6,20 @@ module API
'git-upload-pack' => :ssh_upload_pack 'git-upload-pack' => :ssh_upload_pack
}.freeze }.freeze
attr_reader :redirected_path
# rubocop:disable Cop/ModuleWithInstanceVariables
def wiki? def wiki?
set_project unless defined?(@wiki) set_project unless defined?(@wiki)
@wiki @wiki
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def project def project
set_project unless defined?(@project) set_project unless defined?(@project)
@project @project
end end
def redirected_path
@redirected_path
end
def ssh_authentication_abilities def ssh_authentication_abilities
[ [
:read_project, :read_project,
......
...@@ -21,7 +21,6 @@ module API ...@@ -21,7 +21,6 @@ module API
forbidden! unless current_runner forbidden! unless current_runner
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def current_runner def current_runner
@runner ||= ::Ci::Runner.find_by_token(params[:token].to_s) @runner ||= ::Ci::Runner.find_by_token(params[:token].to_s)
end end
......
# Module providing methods for dealing with separating a tree-ish string and a # Module providing methods for dealing with separating a tree-ish string and a
# file path string when combined in a request parameter # file path string when combined in a request parameter
# rubocop:disable Cop/ModuleWithInstanceVariables
module ExtractsPath module ExtractsPath
# Raised when given an invalid file path # Raised when given an invalid file path
InvalidPathError = Class.new(StandardError) InvalidPathError = Class.new(StandardError)
...@@ -38,6 +37,7 @@ module ExtractsPath ...@@ -38,6 +37,7 @@ module ExtractsPath
# #
# Returns an Array where the first value is the tree-ish and the second is the # Returns an Array where the first value is the tree-ish and the second is the
# path # path
# rubocop:disable Cop/ModuleWithInstanceVariables
def extract_ref(id) def extract_ref(id)
pair = ['', ''] pair = ['', '']
...@@ -105,6 +105,7 @@ module ExtractsPath ...@@ -105,6 +105,7 @@ module ExtractsPath
# #
# Automatically renders `not_found!` if a valid tree path could not be # Automatically renders `not_found!` if a valid tree path could not be
# resolved (e.g., when a user inserts an invalid path or ref). # resolved (e.g., when a user inserts an invalid path or ref).
# rubocop:disable Cop/ModuleWithInstanceVariables
def assign_ref_vars def assign_ref_vars
# assign allowed options # assign allowed options
allowed_options = ["filter_ref"] allowed_options = ["filter_ref"]
...@@ -133,6 +134,7 @@ module ExtractsPath ...@@ -133,6 +134,7 @@ module ExtractsPath
render_404 render_404
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def tree def tree
@tree ||= @repo.tree(@commit.id, @path) @tree ||= @repo.tree(@commit.id, @path)
end end
...@@ -146,6 +148,7 @@ module ExtractsPath ...@@ -146,6 +148,7 @@ module ExtractsPath
id id
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def ref_names def ref_names
return [] unless @project return [] unless @project
......
...@@ -10,7 +10,6 @@ module Gitlab ...@@ -10,7 +10,6 @@ module Gitlab
.transform_keys { |date| date.strftime(@format) } .transform_keys { |date| date.strftime(@format) }
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def interval_step def interval_step
@interval_step ||= 1.day @interval_step ||= 1.day
end end
...@@ -30,7 +29,6 @@ module Gitlab ...@@ -30,7 +29,6 @@ module Gitlab
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def interval_step def interval_step
@interval_step ||= 1.month @interval_step ||= 1.month
end end
......
...@@ -13,7 +13,6 @@ module Gitlab ...@@ -13,7 +13,6 @@ module Gitlab
# script: ... # script: ...
# artifacts: ... # artifacts: ...
# #
# rubocop:disable Cop/ModuleWithInstanceVariables
module Configurable module Configurable
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -25,6 +24,7 @@ module Gitlab ...@@ -25,6 +24,7 @@ module Gitlab
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def compose!(deps = nil) def compose!(deps = nil)
return unless valid? return unless valid?
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module Ci module Ci
class Config class Config
...@@ -13,6 +12,7 @@ module Gitlab ...@@ -13,6 +12,7 @@ module Gitlab
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def errors def errors
@validator.messages + descendants.flat_map(&:errors) @validator.messages + descendants.flat_map(&:errors)
end end
......
...@@ -5,7 +5,6 @@ module Gitlab ...@@ -5,7 +5,6 @@ module Gitlab
"ci_" "ci_"
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def model_name def model_name
@model_name ||= ActiveModel::Name.new(self, nil, self.name.split("::").last) @model_name ||= ActiveModel::Name.new(self, nil, self.name.split("::").last)
end end
......
...@@ -7,11 +7,11 @@ module Gitlab ...@@ -7,11 +7,11 @@ module Gitlab
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def base_query def base_query
@base_query ||= stage_query @base_query ||= stage_query
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def stage_query def stage_query
query = mr_closing_issues_table.join(issue_table).on(issue_table[:id].eq(mr_closing_issues_table[:issue_id])) 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])) .join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id]))
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module CycleAnalytics module CycleAnalytics
module ProductionHelper module ProductionHelper
# rubocop:disable Cop/ModuleWithInstanceVariables
def stage_query 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]))
end end
......
...@@ -12,7 +12,6 @@ module Gitlab ...@@ -12,7 +12,6 @@ module Gitlab
raise NotImplementedError raise NotImplementedError
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def message def message
@message ||= process_message @message ||= process_message
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module Emoji module Emoji
extend self extend self
...@@ -32,8 +31,7 @@ module Gitlab ...@@ -32,8 +31,7 @@ module Gitlab
end end
def emoji_unicode_version(name) 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 end
def normalize_emoji_name(name) def normalize_emoji_name(name)
...@@ -57,5 +55,12 @@ module Gitlab ...@@ -57,5 +55,12 @@ module Gitlab
ActionController::Base.helpers.content_tag('gl-emoji', emoji_info['moji'], title: emoji_info['description'], data: data) ActionController::Base.helpers.content_tag('gl-emoji', emoji_info['moji'], title: emoji_info['description'], data: data)
end 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
end end
...@@ -52,7 +52,6 @@ module Gitlab ...@@ -52,7 +52,6 @@ module Gitlab
end end
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def identification_cache def identification_cache
@identification_cache ||= { @identification_cache ||= {
email: {}, email: {},
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module Metrics module Metrics
module InfluxDb module InfluxDb
...@@ -150,6 +149,7 @@ module Gitlab ...@@ -150,6 +149,7 @@ module Gitlab
# When enabled this should be set before being used as the usual pattern # When enabled this should be set before being used as the usual pattern
# "@foo ||= bar" is _not_ thread-safe. # "@foo ||= bar" is _not_ thread-safe.
# rubocop:disable Cop/ModuleWithInstanceVariables
def pool def pool
if influx_metrics_enabled? if influx_metrics_enabled?
if @pool.nil? if @pool.nil?
......
require 'prometheus/client' require 'prometheus/client'
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module Metrics module Metrics
module Prometheus module Prometheus
...@@ -14,6 +13,7 @@ module Gitlab ...@@ -14,6 +13,7 @@ module Gitlab
::File.writable?(multiprocess_files_dir) ::File.writable?(multiprocess_files_dir)
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def prometheus_metrics_enabled? def prometheus_metrics_enabled?
return @prometheus_metrics_enabled if defined?(@prometheus_metrics_enabled) return @prometheus_metrics_enabled if defined?(@prometheus_metrics_enabled)
......
...@@ -26,7 +26,6 @@ module Gitlab ...@@ -26,7 +26,6 @@ module Gitlab
load_yaml_file&.map(&:deep_symbolize_keys).freeze load_yaml_file&.map(&:deep_symbolize_keys).freeze
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def load_yaml_file def load_yaml_file
@loaded_yaml_file ||= YAML.load_file(Rails.root.join('config/prometheus/additional_metrics.yml')) @loaded_yaml_file ||= YAML.load_file(Rails.root.join('config/prometheus/additional_metrics.yml'))
end end
......
...@@ -56,7 +56,6 @@ module Gitlab ...@@ -56,7 +56,6 @@ module Gitlab
query query
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def available_metrics def available_metrics
@available_metrics ||= client_label_values || [] @available_metrics ||= client_label_values || []
end end
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module Regex module Regex
extend self extend self
......
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
module SlashCommands module SlashCommands
module Presenters module Presenters
...@@ -11,14 +10,17 @@ module Gitlab ...@@ -11,14 +10,17 @@ module Gitlab
issuable.open? ? 'Open' : 'Closed' issuable.open? ? 'Open' : 'Closed'
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def project def project
@resource.project @resource.project
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def author def author
@resource.author @resource.author
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def fields def fields
[ [
{ {
......
...@@ -72,7 +72,6 @@ module Gitlab ...@@ -72,7 +72,6 @@ module Gitlab
private private
# rubocop:disable Cop/ModuleWithInstanceVariables
def default_id def default_id
@default_id ||= begin @default_id ||= begin
id = Gitlab.config.gitlab.default_theme.to_i id = Gitlab.config.gitlab.default_theme.to_i
......
require 'rainbow/ext/string' require 'rainbow/ext/string'
# rubocop:disable Cop/ModuleWithInstanceVariables
module Gitlab module Gitlab
TaskFailedError = Class.new(StandardError) TaskFailedError = Class.new(StandardError)
TaskAbortedByUserError = Class.new(StandardError) TaskAbortedByUserError = Class.new(StandardError)
...@@ -105,6 +104,7 @@ module Gitlab ...@@ -105,6 +104,7 @@ module Gitlab
Gitlab.config.gitlab.user Gitlab.config.gitlab.user
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def gitlab_user? def gitlab_user?
return @is_gitlab_user unless @is_gitlab_user.nil? return @is_gitlab_user unless @is_gitlab_user.nil?
...@@ -112,6 +112,7 @@ module Gitlab ...@@ -112,6 +112,7 @@ module Gitlab
@is_gitlab_user = current_user == gitlab_user @is_gitlab_user = current_user == gitlab_user
end end
# rubocop:disable Cop/ModuleWithInstanceVariables
def warn_user_is_not_gitlab def warn_user_is_not_gitlab
return if @warned_user_not_gitlab return if @warned_user_not_gitlab
......
...@@ -3,7 +3,6 @@ module QA ...@@ -3,7 +3,6 @@ module QA
module Namespace module Namespace
extend self extend self
# rubocop:disable Cop/ModuleWithInstanceVariables
def time def time
@time ||= Time.now @time ||= Time.now
end end
......
...@@ -45,11 +45,29 @@ module RuboCop ...@@ -45,11 +45,29 @@ module RuboCop
def check_method_definition(node) def check_method_definition(node)
node.each_child_node(:def) do |definition| node.each_child_node(:def) do |definition|
definition.each_descendant(:ivar, :ivasgn) do |offense| # We allow this pattern:
add_offense(offense, :expression) # 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 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 end
end end
...@@ -19,12 +19,20 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do ...@@ -19,12 +19,20 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
end end
end end
shared_examples('not registering offense') do
it 'does not register offenses' do
inspect_source(cop, source)
expect(cop.offenses).to be_empty
end
end
context 'when source is a regular module' do context 'when source is a regular module' do
let(:source) do let(:source) do
<<~RUBY <<~RUBY
module M module M
def f def f
@f ||= true @f = true
end end
end end
RUBY RUBY
...@@ -59,7 +67,7 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do ...@@ -59,7 +67,7 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
module N module N
module M module M
def f def f
@f ||= true @f = true
end end
def g def g
...@@ -79,39 +87,86 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do ...@@ -79,39 +87,86 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
it_behaves_like 'registering offense' it_behaves_like 'registering offense'
end end
context 'when source is offending but it is a rails helper' do context 'with regular ivar assignment' do
before do let(:source) do
allow(cop).to receive(:rails_helper?).and_return(true) <<~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 end
it 'does not register offenses' do context 'when source is offending but it is a rails mailer' do
inspect_source(cop, <<~RUBY) 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 module M
def f def f
@f ||= true @f ||= true
end end
end end
RUBY RUBY
expect(cop.offenses).to be_empty
end end
it_behaves_like 'not registering offense'
end end
context 'when source is offending but it is a rails mailer' do context 'when source is using simple or ivar assignment with other ivar' do
before do let(:source) do
allow(cop).to receive(:rails_mailer?).and_return(true) <<~RUBY
module M
def f
@f ||= g(@g)
end
end
RUBY
end end
it 'does not register offenses' do let(:offending_lines) { [3] }
inspect_source(cop, <<~RUBY)
it_behaves_like 'registering offense'
end
context 'when source is using or ivar assignment with something else' do
let(:source) do
<<~RUBY
module M module M
def f def f
@f = true @f ||= true
@f.to_s
end end
end end
RUBY RUBY
expect(cop.offenses).to be_empty
end end
let(:offending_lines) { [3, 4] }
it_behaves_like 'registering offense'
end end
end end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment