diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-08-30 11:56:07 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-08-30 11:56:07 +0300 |
commit | f474f16faed3cbf1c5c113bfe4227109b76ffb3e (patch) | |
tree | 0641af666fe5bc17a9ed55a9ddabc46a9647dd61 | |
parent | f4d04ee9aaaead634543ee024d275f20edbe563e (diff) | |
parent | 4a37cd0dc08ccef417a423b3a9a91b33c26c4199 (diff) |
Merge branch '50414-rubocop-rule-to-enforce-class-methods-over-module' into 'master'
Resolve "Make a Rubocop cop to prefer class_methods over ClassMethods for ActiveSupport::Concern"
Closes #50414
See merge request gitlab-org/gitlab-ce!21379
29 files changed, 202 insertions, 25 deletions
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 7f6d48d972c..4e15b60ccd1 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -26,7 +26,7 @@ module AtomicInternalId extend ActiveSupport::Concern - module ClassMethods + class_methods do def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName # We require init here to retain the ability to recalculate in the absence of a # InternaLId record (we may delete records in `internal_ids` for example). diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index 4200253053a..6f29c92d176 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -12,7 +12,7 @@ module Awardable end end - module ClassMethods + class_methods do def awarded(user, name) sql = <<~EOL EXISTS ( diff --git a/app/models/concerns/case_sensitivity.rb b/app/models/concerns/case_sensitivity.rb index 0ba542b75ab..6e80365ee5b 100644 --- a/app/models/concerns/case_sensitivity.rb +++ b/app/models/concerns/case_sensitivity.rb @@ -4,7 +4,7 @@ module CaseSensitivity extend ActiveSupport::Concern - module ClassMethods + class_methods do # Queries the given columns regardless of the casing used. # # Unlike other ActiveRecord methods this method only operates on a Hash. diff --git a/app/models/concerns/each_batch.rb b/app/models/concerns/each_batch.rb index a9e14cb55eb..8cf0b8b154d 100644 --- a/app/models/concerns/each_batch.rb +++ b/app/models/concerns/each_batch.rb @@ -3,7 +3,7 @@ module EachBatch extend ActiveSupport::Concern - module ClassMethods + class_methods do # Iterates over the rows in a relation in batches, similar to Rails' # `in_batches` but in a more efficient way. # diff --git a/app/models/concerns/ignorable_column.rb b/app/models/concerns/ignorable_column.rb index 2b074c1921c..5c1f7dfcd2a 100644 --- a/app/models/concerns/ignorable_column.rb +++ b/app/models/concerns/ignorable_column.rb @@ -14,7 +14,7 @@ module IgnorableColumn extend ActiveSupport::Concern - module ClassMethods + class_methods do def columns super.reject { |column| ignored_columns.include?(column.name) } end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e8072145551..f881ce2321c 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -118,7 +118,7 @@ module Issuable end end - module ClassMethods + class_methods do # Searches for records with a matching title. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. diff --git a/app/models/concerns/loaded_in_group_list.rb b/app/models/concerns/loaded_in_group_list.rb index a2233eb2997..fc15c6d55ed 100644 --- a/app/models/concerns/loaded_in_group_list.rb +++ b/app/models/concerns/loaded_in_group_list.rb @@ -3,7 +3,7 @@ module LoadedInGroupList extend ActiveSupport::Concern - module ClassMethods + class_methods do def with_counts(archived:) selects_including_counts = [ 'namespaces.*', diff --git a/app/models/concerns/manual_inverse_association.rb b/app/models/concerns/manual_inverse_association.rb index d0d781dc15f..e18edd33ba7 100644 --- a/app/models/concerns/manual_inverse_association.rb +++ b/app/models/concerns/manual_inverse_association.rb @@ -3,7 +3,7 @@ module ManualInverseAssociation extend ActiveSupport::Concern - module ClassMethods + class_methods do def manual_inverse_association(association, inverse) define_method(association) do |*args| super(*args).tap do |value| diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 7e7eccb1c27..393607e82c4 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -10,7 +10,7 @@ module Mentionable extend ActiveSupport::Concern - module ClassMethods + class_methods do # Indicate which attributes of the Mentionable to search for GFM references. def attr_mentionable(attr, options = {}) attr = attr.to_s diff --git a/app/models/concerns/optionally_search.rb b/app/models/concerns/optionally_search.rb index dec97b7dee8..4093429e372 100644 --- a/app/models/concerns/optionally_search.rb +++ b/app/models/concerns/optionally_search.rb @@ -3,7 +3,7 @@ module OptionallySearch extend ActiveSupport::Concern - module ClassMethods + class_methods do def search(*) raise( NotImplementedError, diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 1f6c42f3b3a..614c3242874 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -26,7 +26,7 @@ module Participable extend ActiveSupport::Concern - module ClassMethods + class_methods do # Adds a list of participant attributes. Attributes can either be symbols or # Procs. # diff --git a/app/models/concerns/referable.rb b/app/models/concerns/referable.rb index 468eaf68883..58143a32fdc 100644 --- a/app/models/concerns/referable.rb +++ b/app/models/concerns/referable.rb @@ -40,7 +40,7 @@ module Referable end end - module ClassMethods + class_methods do # The character that prefixes the actual reference identifier # # This should be overridden by the including class. diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb index f47e20229f1..16ea330701d 100644 --- a/app/models/concerns/resolvable_note.rb +++ b/app/models/concerns/resolvable_note.rb @@ -20,7 +20,7 @@ module ResolvableNote scope :unresolved, -> { resolvable.where(resolved_at: nil) } end - module ClassMethods + class_methods do # This method must be kept in sync with `#resolve!` def resolve!(current_user) unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id) diff --git a/app/models/concerns/select_for_project_authorization.rb b/app/models/concerns/select_for_project_authorization.rb index 39306179eb8..333c9118aa5 100644 --- a/app/models/concerns/select_for_project_authorization.rb +++ b/app/models/concerns/select_for_project_authorization.rb @@ -3,7 +3,7 @@ module SelectForProjectAuthorization extend ActiveSupport::Concern - module ClassMethods + class_methods do def select_for_project_authorization select("projects.id AS project_id, members.access_level") end diff --git a/app/models/concerns/sha_attribute.rb b/app/models/concerns/sha_attribute.rb index c322c356db2..e51b4e22c96 100644 --- a/app/models/concerns/sha_attribute.rb +++ b/app/models/concerns/sha_attribute.rb @@ -3,7 +3,7 @@ module ShaAttribute extend ActiveSupport::Concern - module ClassMethods + class_methods do def sha_attribute(name) return if ENV['STATIC_VERIFICATION'] diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb index 501bd1bb83c..29e48f0c5f7 100644 --- a/app/models/concerns/sortable.rb +++ b/app/models/concerns/sortable.rb @@ -19,7 +19,7 @@ module Sortable scope :order_name_desc, -> { reorder(Arel::Nodes::Descending.new(arel_table[:name].lower)) } end - module ClassMethods + class_methods do def order_by(method) case method.to_s when 'created_asc' then order_created_asc diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index c6e3dc385fe..3ff4b4046d3 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -3,7 +3,7 @@ module Spammable extend ActiveSupport::Concern - module ClassMethods + class_methods do def attr_spammable(attr, options = {}) spammable_attrs << [attr.to_s, options] end diff --git a/app/models/concerns/strip_attribute.rb b/app/models/concerns/strip_attribute.rb index 344f677a3f3..c9f5ba7793d 100644 --- a/app/models/concerns/strip_attribute.rb +++ b/app/models/concerns/strip_attribute.rb @@ -14,7 +14,7 @@ module StripAttribute extend ActiveSupport::Concern - module ClassMethods + class_methods do def strip_attributes(*attrs) strip_attrs.concat(attrs) end diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index bb06e31641d..d64c2f82a09 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -11,7 +11,7 @@ module ApplicationWorker set_queue end - module ClassMethods + class_methods do def inherited(subclass) subclass.set_queue end diff --git a/app/workers/concerns/waitable_worker.rb b/app/workers/concerns/waitable_worker.rb index d85bc7d1660..27b94a82444 100644 --- a/app/workers/concerns/waitable_worker.rb +++ b/app/workers/concerns/waitable_worker.rb @@ -3,7 +3,7 @@ module WaitableWorker extend ActiveSupport::Concern - module ClassMethods + class_methods do # Schedules multiple jobs and waits for them to be completed. def bulk_perform_and_wait(args_list, timeout: 10) # Short-circuit: it's more efficient to do small numbers of jobs inline diff --git a/changelogs/unreleased/50414-rubocop-rule-to-enforce-class-methods-over-module.yml b/changelogs/unreleased/50414-rubocop-rule-to-enforce-class-methods-over-module.yml new file mode 100644 index 00000000000..1694fb2376d --- /dev/null +++ b/changelogs/unreleased/50414-rubocop-rule-to-enforce-class-methods-over-module.yml @@ -0,0 +1,5 @@ +--- +title: Adds Rubocop rule to enforce class_methods over module ClassMethods +merge_request: 21379 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index c17089759de..8ee7987cfff 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -84,7 +84,7 @@ module API end end - module ClassMethods + class_methods do private def install_error_responders(base) diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb index 6482fd94ab8..9fd79c491c2 100644 --- a/lib/api/projects_relation_builder.rb +++ b/lib/api/projects_relation_builder.rb @@ -2,7 +2,7 @@ module API module ProjectsRelationBuilder extend ActiveSupport::Concern - module ClassMethods + class_methods do def prepare_relation(projects_relation, options = {}) projects_relation = preload_relation(projects_relation, options) execute_batch_counting(projects_relation) diff --git a/lib/gitlab/github_import/representation/expose_attribute.rb b/lib/gitlab/github_import/representation/expose_attribute.rb index c3405759631..d2438ee8094 100644 --- a/lib/gitlab/github_import/representation/expose_attribute.rb +++ b/lib/gitlab/github_import/representation/expose_attribute.rb @@ -6,7 +6,7 @@ module Gitlab module ExposeAttribute extend ActiveSupport::Concern - module ClassMethods + class_methods do # Defines getter methods for the given attribute names. # # Example: diff --git a/lib/gitlab/graphql/mount_mutation.rb b/lib/gitlab/graphql/mount_mutation.rb index 8cab84d7a5f..9048967d4e1 100644 --- a/lib/gitlab/graphql/mount_mutation.rb +++ b/lib/gitlab/graphql/mount_mutation.rb @@ -5,7 +5,7 @@ module Gitlab module MountMutation extend ActiveSupport::Concern - module ClassMethods + class_methods do def mount_mutation(mutation_class) # Using an underscored field name symbol will make `graphql-ruby` # standardize the field name diff --git a/lib/static_model.rb b/lib/static_model.rb index 60e2dd82e4e..44673c2b5f6 100644 --- a/lib/static_model.rb +++ b/lib/static_model.rb @@ -2,7 +2,7 @@ module StaticModel extend ActiveSupport::Concern - module ClassMethods + class_methods do # Used by ActiveRecord's polymorphic association to set object_id def primary_key 'id' diff --git a/rubocop/cop/prefer_class_methods_over_module.rb b/rubocop/cop/prefer_class_methods_over_module.rb new file mode 100644 index 00000000000..0dfa80ccfab --- /dev/null +++ b/rubocop/cop/prefer_class_methods_over_module.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Enforces the use of 'class_methods' instead of 'module ClassMethods' for activesupport concerns. + # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/50414 + # + # @example + # # bad + # module Foo + # extend ActiveSupport::Concern + # + # module ClassMethods + # def a_class_method + # end + # end + # end + # + # # good + # module Foo + # extend ActiveSupport::Concern + # + # class_methods do + # def a_class_method + # end + # end + # end + # + class PreferClassMethodsOverModule < RuboCop::Cop::Cop + include RangeHelp + + MSG = 'Do not use module ClassMethods, use class_methods block instead.' + + def_node_matcher :extend_activesupport_concern?, <<~PATTERN + (:send nil? :extend (:const (:const nil? :ActiveSupport) :Concern)) + PATTERN + + def on_module(node) + add_offense(node) if node.defined_module_name == 'ClassMethods' && module_extends_activesupport_concern?(node) + end + + def autocorrect(node) + lambda do |corrector| + corrector.replace(module_range(node), 'class_methods do') + end + end + + private + + def module_extends_activesupport_concern?(node) + container_module = container_module_of(node) + return false unless container_module + + container_module.descendants.any? do |descendant| + extend_activesupport_concern?(descendant) + end + end + + def container_module_of(node) + while node = node.parent + break if node.type == :module + end + + node + end + + def module_range(node) + module_node, _ = *node + range_between(node.loc.keyword.begin_pos, module_node.source_range.end_pos) + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index eaf421a7235..d823fa4edb1 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -7,6 +7,7 @@ require_relative 'cop/include_sidekiq_worker' require_relative 'cop/avoid_return_from_blocks' require_relative 'cop/avoid_break_from_strong_memoize' require_relative 'cop/line_break_around_conditional_block' +require_relative 'cop/prefer_class_methods_over_module' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' diff --git a/spec/rubocop/cop/prefer_class_methods_over_module_spec.rb b/spec/rubocop/cop/prefer_class_methods_over_module_spec.rb new file mode 100644 index 00000000000..4739f0e6c47 --- /dev/null +++ b/spec/rubocop/cop/prefer_class_methods_over_module_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/prefer_class_methods_over_module' + +describe RuboCop::Cop::PreferClassMethodsOverModule do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags violation when using module ClassMethods' do + expect_offense(<<~RUBY) + module Foo + extend ActiveSupport::Concern + + module ClassMethods + ^^^^^^^^^^^^^^^^^^^ Do not use module ClassMethods, use class_methods block instead. + def a_class_method + end + end + end + RUBY + end + + it "doesn't flag violation when using class_methods" do + expect_no_offenses(<<~RUBY) + module Foo + extend ActiveSupport::Concern + + class_methods do + def a_class_method + end + end + end + RUBY + end + + it "doesn't flag violation when module is not extending ActiveSupport::Concern" do + expect_no_offenses(<<~RUBY) + module Foo + module ClassMethods + def a_class_method + end + end + end + RUBY + end + + it "doesn't flag violation when ClassMethods is used inside a class" do + expect_no_offenses(<<~RUBY) + class Foo + module ClassMethods + def a_class_method + end + end + end + RUBY + end + + it "doesn't flag violation when not using either class_methods or ClassMethods" do + expect_no_offenses(<<~RUBY) + module Foo + extend ActiveSupport::Concern + + def a_method + end + end + RUBY + end + + it 'autocorrects ClassMethods into class_methods' do + source = <<~RUBY + module Foo + extend ActiveSupport::Concern + + module ClassMethods + def a_class_method + end + end + end + RUBY + autocorrected = autocorrect_source(source) + + expected_source = <<~RUBY + module Foo + extend ActiveSupport::Concern + + class_methods do + def a_class_method + end + end + end + RUBY + expect(autocorrected).to eq(expected_source) + end +end |