Introduce PredicateMemoization cop and fix offenses
with StrongMemoize
This commit is contained in:
parent
3fde958f36
commit
4f00a05152
|
@ -1,5 +1,6 @@
|
|||
module ResolvableDiscussion
|
||||
extend ActiveSupport::Concern
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
included do
|
||||
# A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
|
||||
|
@ -31,27 +32,35 @@ module ResolvableDiscussion
|
|||
end
|
||||
|
||||
def resolvable?
|
||||
@resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?)
|
||||
strong_memoize(:resolvable) do
|
||||
potentially_resolvable? && notes.any?(&:resolvable?)
|
||||
end
|
||||
end
|
||||
|
||||
def resolved?
|
||||
@resolved ||= resolvable? && notes.none?(&:to_be_resolved?)
|
||||
strong_memoize(:resolved) do
|
||||
resolvable? && notes.none?(&:to_be_resolved?)
|
||||
end
|
||||
end
|
||||
|
||||
def first_note
|
||||
@first_note ||= notes.first
|
||||
notes.first
|
||||
end
|
||||
|
||||
def first_note_to_resolve
|
||||
return unless resolvable?
|
||||
|
||||
@first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
||||
strong_memoize(:first_note_to_resolve) do
|
||||
notes.find(&:to_be_resolved?)
|
||||
end
|
||||
end
|
||||
|
||||
def last_resolved_note
|
||||
return unless resolved?
|
||||
|
||||
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
||||
strong_memoize(:last_resolved_note) do
|
||||
resolved_notes.sort_by(&:resolved_at).last
|
||||
end
|
||||
end
|
||||
|
||||
def resolved_notes
|
||||
|
|
|
@ -20,6 +20,7 @@ class Project < ActiveRecord::Base
|
|||
include GroupDescendant
|
||||
include Gitlab::SQL::Pattern
|
||||
include DeploymentPlatform
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
extend Gitlab::ConfigHelper
|
||||
extend Gitlab::CurrentSettings
|
||||
|
@ -993,9 +994,13 @@ class Project < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def repo_exists?
|
||||
@repo_exists ||= repository.exists?
|
||||
rescue
|
||||
@repo_exists = false
|
||||
strong_memoize(:repo_exists) do
|
||||
begin
|
||||
repository.exists?
|
||||
rescue
|
||||
false
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def root_ref?(branch)
|
||||
|
|
|
@ -895,15 +895,18 @@ class Repository
|
|||
branch = Gitlab::Git::Branch.find(self, branch_or_name)
|
||||
|
||||
if branch
|
||||
@root_ref_sha ||= commit(root_ref).sha
|
||||
same_head = branch.target == @root_ref_sha
|
||||
merged = ancestor?(branch.target, @root_ref_sha)
|
||||
same_head = branch.target == root_ref_sha
|
||||
merged = ancestor?(branch.target, root_ref_sha)
|
||||
!same_head && merged
|
||||
else
|
||||
nil
|
||||
end
|
||||
end
|
||||
|
||||
def root_ref_sha
|
||||
@root_ref_sha ||= commit(root_ref).sha
|
||||
end
|
||||
|
||||
delegate :merged_branch_names, to: :raw_repository
|
||||
|
||||
def merge_base(first_commit_id, second_commit_id)
|
||||
|
|
|
@ -7,6 +7,7 @@ module Gitlab
|
|||
class PrepareUntrackedUploads # rubocop:disable Metrics/ClassLength
|
||||
# For bulk_queue_background_migration_jobs_by_range
|
||||
include Database::MigrationHelpers
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
FIND_BATCH_SIZE = 500
|
||||
RELATIVE_UPLOAD_DIR = "uploads".freeze
|
||||
|
@ -142,7 +143,9 @@ module Gitlab
|
|||
end
|
||||
|
||||
def postgresql?
|
||||
@postgresql ||= Gitlab::Database.postgresql?
|
||||
strong_memoize(:postgresql) do
|
||||
Gitlab::Database.postgresql?
|
||||
end
|
||||
end
|
||||
|
||||
def can_bulk_insert_and_ignore_duplicates?
|
||||
|
@ -150,8 +153,9 @@ module Gitlab
|
|||
end
|
||||
|
||||
def postgresql_pre_9_5?
|
||||
@postgresql_pre_9_5 ||= postgresql? &&
|
||||
Gitlab::Database.version.to_f < 9.5
|
||||
strong_memoize(:postgresql_pre_9_5) do
|
||||
postgresql? && Gitlab::Database.version.to_f < 9.5
|
||||
end
|
||||
end
|
||||
|
||||
def schedule_populate_untracked_uploads_jobs
|
||||
|
|
|
@ -1,6 +1,8 @@
|
|||
module Gitlab
|
||||
module BareRepositoryImport
|
||||
class Repository
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
attr_reader :group_path, :project_name, :repo_path
|
||||
|
||||
def initialize(root_path, repo_path)
|
||||
|
@ -41,11 +43,15 @@ module Gitlab
|
|||
private
|
||||
|
||||
def wiki?
|
||||
@wiki ||= repo_path.end_with?('.wiki.git')
|
||||
strong_memoize(:wiki) do
|
||||
repo_path.end_with?('.wiki.git')
|
||||
end
|
||||
end
|
||||
|
||||
def hashed?
|
||||
@hashed ||= repo_relative_path.include?('@hashed')
|
||||
strong_memoize(:hashed) do
|
||||
repo_relative_path.include?('@hashed')
|
||||
end
|
||||
end
|
||||
|
||||
def repo_relative_path
|
||||
|
|
|
@ -3,6 +3,8 @@ module Gitlab
|
|||
module Pipeline
|
||||
module Chain
|
||||
class Skip < Chain::Base
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i
|
||||
|
||||
def perform!
|
||||
|
@ -24,7 +26,9 @@ module Gitlab
|
|||
def commit_message_skips_ci?
|
||||
return false unless @pipeline.git_commit_message
|
||||
|
||||
@skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
|
||||
strong_memoize(:commit_message_skips_ci) do
|
||||
!!(@pipeline.git_commit_message =~ SKIP_PATTERN)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -2,6 +2,8 @@ module Gitlab
|
|||
module Ci
|
||||
module Stage
|
||||
class Seed
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
attr_reader :pipeline
|
||||
|
||||
delegate :project, to: :pipeline
|
||||
|
@ -50,7 +52,9 @@ module Gitlab
|
|||
private
|
||||
|
||||
def protected_ref?
|
||||
@protected_ref ||= project.protected_for?(pipeline.ref)
|
||||
strong_memoize(:protected_ref) do
|
||||
project.protected_for?(pipeline.ref)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -14,6 +14,8 @@ module Gitlab
|
|||
# puts label.name
|
||||
# end
|
||||
class Client
|
||||
include ::Gitlab::Utils::StrongMemoize
|
||||
|
||||
attr_reader :octokit
|
||||
|
||||
# A single page of data and the corresponding page number.
|
||||
|
@ -173,7 +175,9 @@ module Gitlab
|
|||
end
|
||||
|
||||
def rate_limiting_enabled?
|
||||
@rate_limiting_enabled ||= api_endpoint.include?('.github.com')
|
||||
strong_memoize(:rate_limiting_enabled) do
|
||||
api_endpoint.include?('.github.com')
|
||||
end
|
||||
end
|
||||
|
||||
def api_endpoint
|
||||
|
|
|
@ -16,8 +16,10 @@ module Gitlab
|
|||
def can_do_action?(action)
|
||||
return false unless can_access_git?
|
||||
|
||||
@permission_cache ||= {}
|
||||
@permission_cache[action] ||= user.can?(action, project)
|
||||
permission_cache[action] =
|
||||
permission_cache.fetch(action) do
|
||||
user.can?(action, project)
|
||||
end
|
||||
end
|
||||
|
||||
def cannot_do_action?(action)
|
||||
|
@ -88,6 +90,10 @@ module Gitlab
|
|||
|
||||
private
|
||||
|
||||
def permission_cache
|
||||
@permission_cache ||= {}
|
||||
end
|
||||
|
||||
def can_access_git?
|
||||
user && user.can?(:access_git)
|
||||
end
|
||||
|
|
|
@ -0,0 +1,39 @@
|
|||
module RuboCop
|
||||
module Cop
|
||||
module Gitlab
|
||||
class PredicateMemoization < RuboCop::Cop::Cop
|
||||
MSG = <<~EOL.freeze
|
||||
Avoid using `@value ||= query` inside predicate methods in order to
|
||||
properly memoize `false` or `nil` values.
|
||||
https://docs.gitlab.com/ee/development/utilities.html#strongmemoize
|
||||
EOL
|
||||
|
||||
def on_def(node)
|
||||
return unless predicate_method?(node)
|
||||
|
||||
select_offenses(node).each do |offense|
|
||||
add_offense(offense, location: :expression)
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def predicate_method?(node)
|
||||
node.method_name.to_s.end_with?('?')
|
||||
end
|
||||
|
||||
def or_ivar_assignment?(or_assignment)
|
||||
lhs = or_assignment.each_child_node.first
|
||||
|
||||
lhs.ivasgn_type?
|
||||
end
|
||||
|
||||
def select_offenses(node)
|
||||
node.each_descendant(:or_asgn).select do |or_assignment|
|
||||
or_ivar_assignment?(or_assignment)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1,4 +1,5 @@
|
|||
require_relative 'cop/gitlab/module_with_instance_variables'
|
||||
require_relative 'cop/gitlab/predicate_memoization'
|
||||
require_relative 'cop/include_sidekiq_worker'
|
||||
require_relative 'cop/line_break_around_conditional_block'
|
||||
require_relative 'cop/migration/add_column'
|
||||
|
|
|
@ -0,0 +1,86 @@
|
|||
require 'spec_helper'
|
||||
require 'rubocop'
|
||||
require 'rubocop/rspec/support'
|
||||
require_relative '../../../../rubocop/cop/gitlab/predicate_memoization'
|
||||
|
||||
describe RuboCop::Cop::Gitlab::PredicateMemoization do
|
||||
include CopHelper
|
||||
|
||||
subject(:cop) { described_class.new }
|
||||
|
||||
shared_examples('registering offense') do |options|
|
||||
let(:offending_lines) { options[:offending_lines] }
|
||||
|
||||
it 'registers an offense when a predicate method is memoizing via ivar' do
|
||||
inspect_source(source)
|
||||
|
||||
aggregate_failures do
|
||||
expect(cop.offenses.size).to eq(offending_lines.size)
|
||||
expect(cop.offenses.map(&:line)).to eq(offending_lines)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples('not registering offense') do
|
||||
it 'does not register offenses' do
|
||||
inspect_source(source)
|
||||
|
||||
expect(cop.offenses).to be_empty
|
||||
end
|
||||
end
|
||||
|
||||
context 'when source is a predicate method memoizing via ivar' do
|
||||
it_behaves_like 'registering offense', offending_lines: [3] do
|
||||
let(:source) do
|
||||
<<~RUBY
|
||||
class C
|
||||
def really?
|
||||
@really ||= true
|
||||
end
|
||||
end
|
||||
RUBY
|
||||
end
|
||||
end
|
||||
|
||||
it_behaves_like 'registering offense', offending_lines: [4] do
|
||||
let(:source) do
|
||||
<<~RUBY
|
||||
class C
|
||||
def really?
|
||||
value = true
|
||||
@really ||= value
|
||||
end
|
||||
end
|
||||
RUBY
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when source is a predicate method using ivar with assignment' do
|
||||
it_behaves_like 'not registering offense' do
|
||||
let(:source) do
|
||||
<<~RUBY
|
||||
class C
|
||||
def really?
|
||||
@really = true
|
||||
end
|
||||
end
|
||||
RUBY
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when source is a regular method memoizing via ivar' do
|
||||
it_behaves_like 'not registering offense' do
|
||||
let(:source) do
|
||||
<<~RUBY
|
||||
class C
|
||||
def really
|
||||
@really ||= true
|
||||
end
|
||||
end
|
||||
RUBY
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue