diff --git a/.rubocop.yml b/.rubocop.yml index 93fa921acd1..4f05d5be2d7 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -24,6 +24,11 @@ Naming/PredicateName: - def_node_matcher - def_node_search +Style/AccessorGrouping: + Exclude: + - lib/rubocop/formatter/base_formatter.rb + - lib/rubocop/cop/offense.rb + Style/FormatStringToken: # Because we parse a lot of source codes from strings. Percent arrays # look like unannotated format string tokens to this cop. diff --git a/CHANGELOG.md b/CHANGELOG.md index 22f73d71152..9b5629a1b52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#7868](https://github.com/rubocop-hq/rubocop/pull/7868): `Cop::Base` is the new recommended base class for cops. ([@marcandre][]) +* [#3983](https://github.com/rubocop-hq/rubocop/issues/3983): Add new `Style/AccessorGrouping` cop. ([@fatkodima][]) * [#8244](https://github.com/rubocop-hq/rubocop/pull/8244): Add new `Style/BisectedAttrAccessor` cop. ([@fatkodima][]) * [#7458](https://github.com/rubocop-hq/rubocop/issues/7458): Add new `AsciiConstants` option for `Naming/AsciiIdentifiers`. ([@fatkodima][]) * [#7373](https://github.com/rubocop-hq/rubocop/issues/7373): Add new `Style/RedundantAssignment` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index 3f08bae0abe..469e3bdefdc 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2299,6 +2299,17 @@ Style/AccessModifierDeclarations: - group AllowModifiersOnSymbols: true +Style/AccessorGrouping: + Description: 'Checks for grouping of accessors in `class` and `module` bodies.' + Enabled: 'pending' + VersionAdded: '0.87' + EnforcedStyle: grouped + SupportedStyles: + # separated: each accessor goes in a separate statement. + # grouped: accessors are grouped into a single statement. + - separated + - grouped + Style/Alias: Description: 'Use alias instead of alias_method.' StyleGuide: '#alias-method-lexically' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 634d797cfcb..08a7310745d 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -313,6 +313,7 @@ In the following section you find all available cops: === Department xref:cops_style.adoc[Style] * xref:cops_style.adoc#styleaccessmodifierdeclarations[Style/AccessModifierDeclarations] +* xref:cops_style.adoc#styleaccessorgrouping[Style/AccessorGrouping] * xref:cops_style.adoc#stylealias[Style/Alias] * xref:cops_style.adoc#styleandor[Style/AndOr] * xref:cops_style.adoc#stylearrayjoin[Style/ArrayJoin] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index 7456cbe08bc..dd3fe61c0df 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -104,6 +104,66 @@ end | Boolean |=== +== Style/AccessorGrouping + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| Yes +| 0.87 +| - +|=== + +This cop checks for grouping of accessors in `class` and `module` bodies. +By default it enforces accessors to be placed in grouped declarations, +but it can be configured to enforce separating them in multiple declarations. + +=== Examples + +==== EnforcedStyle: grouped (default) + +[source,ruby] +---- +# bad +class Foo + attr_reader :bar + attr_reader :baz +end + +# good +class Foo + attr_reader :bar, :baz +end +---- + +==== EnforcedStyle: separated + +[source,ruby] +---- +# bad +class Foo + attr_reader :bar, :baz +end + +# good +class Foo + attr_reader :bar + attr_reader :baz +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| EnforcedStyle +| `grouped` +| `separated`, `grouped` +|=== + == Style/Alias |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 9045ab19861..8be74740c3a 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -356,6 +356,7 @@ require_relative 'rubocop/cop/naming/variable_number' require_relative 'rubocop/cop/style/access_modifier_declarations' +require_relative 'rubocop/cop/style/accessor_grouping' require_relative 'rubocop/cop/style/alias' require_relative 'rubocop/cop/style/and_or' require_relative 'rubocop/cop/style/array_join' diff --git a/lib/rubocop/cop/layout/hash_alignment.rb b/lib/rubocop/cop/layout/hash_alignment.rb index c565117f7bd..e2548909426 100644 --- a/lib/rubocop/cop/layout/hash_alignment.rb +++ b/lib/rubocop/cop/layout/hash_alignment.rb @@ -219,8 +219,7 @@ def autocorrect(node) correct_node(node, delta) end - attr_accessor :offences_by - attr_accessor :column_deltas + attr_accessor :offences_by, :column_deltas private diff --git a/lib/rubocop/cop/style/accessor_grouping.rb b/lib/rubocop/cop/style/accessor_grouping.rb new file mode 100644 index 00000000000..b9067916358 --- /dev/null +++ b/lib/rubocop/cop/style/accessor_grouping.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # This cop checks for grouping of accessors in `class` and `module` bodies. + # By default it enforces accessors to be placed in grouped declarations, + # but it can be configured to enforce separating them in multiple declarations. + # + # @example EnforcedStyle: grouped (default) + # # bad + # class Foo + # attr_reader :bar + # attr_reader :baz + # end + # + # # good + # class Foo + # attr_reader :bar, :baz + # end + # + # @example EnforcedStyle: separated + # # bad + # class Foo + # attr_reader :bar, :baz + # end + # + # # good + # class Foo + # attr_reader :bar + # attr_reader :baz + # end + # + class AccessorGrouping < Cop + include ConfigurableEnforcedStyle + + GROUPED_MSG = 'Group together all `%s` attributes.' + SEPARATED_MSG = 'Use one attribute per `%s`.' + + ACCESSOR_METHODS = %i[attr_reader attr_writer attr_accessor attr].freeze + + def on_class(node) + class_send_elements(node).each do |macro| + next unless accessor?(macro) + + check(macro) + end + end + alias on_module on_class + + def autocorrect(node) + lambda do |corrector| + corrector.replace(node, correction(node)) + end + end + + private + + def class_send_elements(class_node) + class_def = class_node.body + + if !class_def || class_def.def_type? + [] + elsif class_def.send_type? + [class_def] + else + class_def.each_child_node(:send).to_a + end + end + + def accessor?(send_node) + send_node.macro? && ACCESSOR_METHODS.include?(send_node.method_name) + end + + def check(send_node) + if grouped_style? && sibling_accessors(send_node).size > 1 + add_offense(send_node) + elsif separated_style? && send_node.arguments.size > 1 + add_offense(send_node) + end + end + + def grouped_style? + style == :grouped + end + + def separated_style? + style == :separated + end + + def sibling_accessors(send_node) + send_node.parent.each_child_node(:send).select do |sibling| + sibling.macro? && sibling.method?(send_node.method_name) + end + end + + def message(send_node) + msg = grouped_style? ? GROUPED_MSG : SEPARATED_MSG + format(msg, accessor: send_node.method_name) + end + + def correction(node) + if grouped_style? + accessors = sibling_accessors(node) + if node == accessors.first + group_accessors(node, accessors) + else + '' + end + else + separate_accessors(node) + end + end + + def group_accessors(node, accessors) + accessor_names = accessors.flat_map do |accessor| + accessor.arguments.map(&:source) + end + + "#{node.method_name} #{accessor_names.join(', ')}" + end + + def separate_accessors(node) + node.arguments.map do |arg| + if arg == node.arguments.first + "#{node.method_name} #{arg.source}" + else + indent = ' ' * node.loc.column + "#{indent}#{node.method_name} #{arg.source}" + end + end.join("\n") + end + end + end + end +end diff --git a/lib/rubocop/cop/utils/format_string.rb b/lib/rubocop/cop/utils/format_string.rb index dd99d5e27e0..96de5f8f18f 100644 --- a/lib/rubocop/cop/utils/format_string.rb +++ b/lib/rubocop/cop/utils/format_string.rb @@ -41,8 +41,7 @@ class FormatString # # @see https://ruby-doc.org/core-2.6.3/Kernel.html#method-i-format class FormatSequence - attr_reader :begin_pos, :end_pos - attr_reader :flags, :width, :precision, :name, :type + attr_reader :begin_pos, :end_pos, :flags, :width, :precision, :name, :type def initialize(match) @source = match[0] diff --git a/lib/rubocop/rake_task.rb b/lib/rubocop/rake_task.rb index 3702c050635..809be7f0057 100644 --- a/lib/rubocop/rake_task.rb +++ b/lib/rubocop/rake_task.rb @@ -12,13 +12,7 @@ module RuboCop # Use global Rake namespace here to avoid namespace issues with custom # rubocop-rake tasks class RakeTask < ::Rake::TaskLib - attr_accessor :name - attr_accessor :verbose - attr_accessor :fail_on_error - attr_accessor :patterns - attr_accessor :formatters - attr_accessor :requires - attr_accessor :options + attr_accessor :name, :verbose, :fail_on_error, :patterns, :formatters, :requires, :options def initialize(name = :rubocop, *args, &task_block) setup_ivars(name) diff --git a/spec/rubocop/cop/style/accessor_grouping_spec.rb b/spec/rubocop/cop/style/accessor_grouping_spec.rb new file mode 100644 index 00000000000..d9aaa7ab3f3 --- /dev/null +++ b/spec/rubocop/cop/style/accessor_grouping_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::AccessorGrouping, :config do + subject(:cop) { described_class.new(config) } + + context 'when EnforcedStyle is grouped' do + let(:cop_config) do + { 'EnforcedStyle' => 'grouped' } + end + + it 'registers an offense and corrects when using separated accessors' do + expect_offense(<<~RUBY) + class Foo + attr_reader :bar1 + ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes. + attr_reader :bar2 + ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes. + attr_accessor :quux + attr_reader :bar3, :bar4 + ^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes. + other_macro :zoo + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_reader :bar1, :bar2, :bar3, :bar4 + + attr_accessor :quux + + other_macro :zoo + end + RUBY + end + + it 'does not register an offense when using grouped accessors' do + expect_no_offenses(<<~RUBY) + class Foo + attr_reader :bar, :baz + end + RUBY + end + end + + context 'when EnforcedStyle is separated' do + let(:cop_config) do + { 'EnforcedStyle' => 'separated' } + end + + it 'registers an offense and corrects when using grouped accessors' do + expect_offense(<<~RUBY) + class Foo + attr_reader :bar, :baz + ^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`. + attr_accessor :quux + other_macro :zoo, :woo + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_reader :bar + attr_reader :baz + attr_accessor :quux + other_macro :zoo, :woo + end + RUBY + end + + it 'does not register an offense when using separated accessors' do + expect_no_offenses(<<~RUBY) + class Foo + attr_reader :bar + attr_reader :baz + end + RUBY + end + end +end