Commit f474f16f authored by Sean McGivern's avatar Sean McGivern

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
parents f4d04ee9 4a37cd0d
...@@ -26,7 +26,7 @@ ...@@ -26,7 +26,7 @@
module AtomicInternalId module AtomicInternalId
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName 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 # 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). # InternaLId record (we may delete records in `internal_ids` for example).
......
...@@ -12,7 +12,7 @@ module Awardable ...@@ -12,7 +12,7 @@ module Awardable
end end
end end
module ClassMethods class_methods do
def awarded(user, name) def awarded(user, name)
sql = <<~EOL sql = <<~EOL
EXISTS ( EXISTS (
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
module CaseSensitivity module CaseSensitivity
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Queries the given columns regardless of the casing used. # Queries the given columns regardless of the casing used.
# #
# Unlike other ActiveRecord methods this method only operates on a Hash. # Unlike other ActiveRecord methods this method only operates on a Hash.
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module EachBatch module EachBatch
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Iterates over the rows in a relation in batches, similar to Rails' # Iterates over the rows in a relation in batches, similar to Rails'
# `in_batches` but in a more efficient way. # `in_batches` but in a more efficient way.
# #
......
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
module IgnorableColumn module IgnorableColumn
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def columns def columns
super.reject { |column| ignored_columns.include?(column.name) } super.reject { |column| ignored_columns.include?(column.name) }
end end
......
...@@ -118,7 +118,7 @@ module Issuable ...@@ -118,7 +118,7 @@ module Issuable
end end
end end
module ClassMethods class_methods do
# Searches for records with a matching title. # Searches for records with a matching title.
# #
# This method uses ILIKE on PostgreSQL and LIKE on MySQL. # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module LoadedInGroupList module LoadedInGroupList
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def with_counts(archived:) def with_counts(archived:)
selects_including_counts = [ selects_including_counts = [
'namespaces.*', 'namespaces.*',
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module ManualInverseAssociation module ManualInverseAssociation
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def manual_inverse_association(association, inverse) def manual_inverse_association(association, inverse)
define_method(association) do |*args| define_method(association) do |*args|
super(*args).tap do |value| super(*args).tap do |value|
......
...@@ -10,7 +10,7 @@ ...@@ -10,7 +10,7 @@
module Mentionable module Mentionable
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Indicate which attributes of the Mentionable to search for GFM references. # Indicate which attributes of the Mentionable to search for GFM references.
def attr_mentionable(attr, options = {}) def attr_mentionable(attr, options = {})
attr = attr.to_s attr = attr.to_s
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module OptionallySearch module OptionallySearch
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def search(*) def search(*)
raise( raise(
NotImplementedError, NotImplementedError,
......
...@@ -26,7 +26,7 @@ ...@@ -26,7 +26,7 @@
module Participable module Participable
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Adds a list of participant attributes. Attributes can either be symbols or # Adds a list of participant attributes. Attributes can either be symbols or
# Procs. # Procs.
# #
......
...@@ -40,7 +40,7 @@ module Referable ...@@ -40,7 +40,7 @@ module Referable
end end
end end
module ClassMethods class_methods do
# The character that prefixes the actual reference identifier # The character that prefixes the actual reference identifier
# #
# This should be overridden by the including class. # This should be overridden by the including class.
......
...@@ -20,7 +20,7 @@ module ResolvableNote ...@@ -20,7 +20,7 @@ module ResolvableNote
scope :unresolved, -> { resolvable.where(resolved_at: nil) } scope :unresolved, -> { resolvable.where(resolved_at: nil) }
end end
module ClassMethods class_methods do
# This method must be kept in sync with `#resolve!` # This method must be kept in sync with `#resolve!`
def resolve!(current_user) def resolve!(current_user)
unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id) unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module SelectForProjectAuthorization module SelectForProjectAuthorization
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def select_for_project_authorization def select_for_project_authorization
select("projects.id AS project_id, members.access_level") select("projects.id AS project_id, members.access_level")
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module ShaAttribute module ShaAttribute
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def sha_attribute(name) def sha_attribute(name)
return if ENV['STATIC_VERIFICATION'] return if ENV['STATIC_VERIFICATION']
......
...@@ -19,7 +19,7 @@ module Sortable ...@@ -19,7 +19,7 @@ module Sortable
scope :order_name_desc, -> { reorder(Arel::Nodes::Descending.new(arel_table[:name].lower)) } scope :order_name_desc, -> { reorder(Arel::Nodes::Descending.new(arel_table[:name].lower)) }
end end
module ClassMethods class_methods do
def order_by(method) def order_by(method)
case method.to_s case method.to_s
when 'created_asc' then order_created_asc when 'created_asc' then order_created_asc
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module Spammable module Spammable
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def attr_spammable(attr, options = {}) def attr_spammable(attr, options = {})
spammable_attrs << [attr.to_s, options] spammable_attrs << [attr.to_s, options]
end end
......
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
module StripAttribute module StripAttribute
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def strip_attributes(*attrs) def strip_attributes(*attrs)
strip_attrs.concat(attrs) strip_attrs.concat(attrs)
end end
......
...@@ -11,7 +11,7 @@ module ApplicationWorker ...@@ -11,7 +11,7 @@ module ApplicationWorker
set_queue set_queue
end end
module ClassMethods class_methods do
def inherited(subclass) def inherited(subclass)
subclass.set_queue subclass.set_queue
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module WaitableWorker module WaitableWorker
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Schedules multiple jobs and waits for them to be completed. # Schedules multiple jobs and waits for them to be completed.
def bulk_perform_and_wait(args_list, timeout: 10) def bulk_perform_and_wait(args_list, timeout: 10)
# Short-circuit: it's more efficient to do small numbers of jobs inline # Short-circuit: it's more efficient to do small numbers of jobs inline
......
---
title: Adds Rubocop rule to enforce class_methods over module ClassMethods
merge_request: 21379
author: Jacopo Beschi @jacopo-beschi
type: added
...@@ -84,7 +84,7 @@ module API ...@@ -84,7 +84,7 @@ module API
end end
end end
module ClassMethods class_methods do
private private
def install_error_responders(base) def install_error_responders(base)
......
...@@ -2,7 +2,7 @@ module API ...@@ -2,7 +2,7 @@ module API
module ProjectsRelationBuilder module ProjectsRelationBuilder
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def prepare_relation(projects_relation, options = {}) def prepare_relation(projects_relation, options = {})
projects_relation = preload_relation(projects_relation, options) projects_relation = preload_relation(projects_relation, options)
execute_batch_counting(projects_relation) execute_batch_counting(projects_relation)
......
...@@ -6,7 +6,7 @@ module Gitlab ...@@ -6,7 +6,7 @@ module Gitlab
module ExposeAttribute module ExposeAttribute
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Defines getter methods for the given attribute names. # Defines getter methods for the given attribute names.
# #
# Example: # Example:
......
...@@ -5,7 +5,7 @@ module Gitlab ...@@ -5,7 +5,7 @@ module Gitlab
module MountMutation module MountMutation
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
def mount_mutation(mutation_class) def mount_mutation(mutation_class)
# Using an underscored field name symbol will make `graphql-ruby` # Using an underscored field name symbol will make `graphql-ruby`
# standardize the field name # standardize the field name
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module StaticModel module StaticModel
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods class_methods do
# Used by ActiveRecord's polymorphic association to set object_id # Used by ActiveRecord's polymorphic association to set object_id
def primary_key def primary_key
'id' 'id'
......
# 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
...@@ -7,6 +7,7 @@ require_relative 'cop/include_sidekiq_worker' ...@@ -7,6 +7,7 @@ require_relative 'cop/include_sidekiq_worker'
require_relative 'cop/avoid_return_from_blocks' require_relative 'cop/avoid_return_from_blocks'
require_relative 'cop/avoid_break_from_strong_memoize' require_relative 'cop/avoid_break_from_strong_memoize'
require_relative 'cop/line_break_around_conditional_block' 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_column'
require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_foreign_key'
require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_concurrent_index'
......
# 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
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