Skip to content

Commit

Permalink
[Fix #8663] Fix issues with Style/ClassMethodsDefinitions (#8687)
Browse files Browse the repository at this point in the history 
Autocorrection was rewritten, which notably moves the offense for def_self style to the sclass itself, in order to properly handle each method within it without getting clobbering errors.

Autocorrection fixes:
* Handle multiple methods within class << self
* Remove extra whitespace
* Fix indentation
* Remove self << class if empty

Co-authored-by: Bozhidar Batsov <[email protected]>
  • Loading branch information
@dvandersluis @bbatsov
dvandersluis and bbatsov authored Sep 15, 2020
1 parent 0d3f5e2 commit 295b9a6
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 38 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
* [#8617](https://github.com/rubocop-hq/rubocop/issues/8617): Fix `Style/HashAsLastArrayItem` to not register an offense when all items in an array are hashes. ([@dvandersluis][])
* [#8500](https://github.com/rubocop-hq/rubocop/issues/8500): Add `in?` to AllowedMethods for `Lint/SafeNavigationChain` cop. ([@tejasbubane][])
* [#8629](https://github.com/rubocop-hq/rubocop/pull/8629): Fix the cache being reusable in CI by using crc32 to calculate file hashes rather than `mtime`, which changes each CI build. ([@dvandersluis][])
* [#8663](https://github.com/rubocop-hq/rubocop/issues/8663): Fix multiple autocorrection bugs with `Style/ClassMethodsDefinitions`. ([@dvandersluis][])
* [#8621](https://github.com/rubocop-hq/rubocop/issues/8621): Add helpful Infinite Loop error message. ([@iSarCasm][])

## 0.90.0 (2020-09-01)
Expand Down
12 changes: 3 additions & 9 deletions lib/rubocop/cop/mixin/comments_help.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,9 @@ module CommentsHelp
include VisibilityHelp

def source_range_with_comment(node)
begin_pos, end_pos =
if node.def_type?
start_node = find_visibility_start(node) || node
end_node = find_visibility_end(node) || node
[begin_pos_with_comment(start_node),
end_position_for(end_node) + 1]
else
[begin_pos_with_comment(node), end_position_for(node)]
end
begin_pos = begin_pos_with_comment(node)
end_pos = end_position_for(node)
end_pos += 1 if node.def_type?

Parser::Source::Range.new(buffer, begin_pos, end_pos)
end
Expand Down
58 changes: 42 additions & 16 deletions lib/rubocop/cop/style/class_methods_definitions.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,19 @@ class ClassMethodsDefinitions < Base
include ConfigurableEnforcedStyle
include CommentsHelp
include VisibilityHelp
include RangeHelp
extend AutoCorrector

MSG = 'Use `%<preferred>s` to define class method.'
MSG = 'Use `%<preferred>s` to define a class method.'
MSG_SCLASS = 'Do not define public methods within class << self.'

def on_sclass(node)
return unless def_self_style?
return unless node.identifier.source == 'self'
return if contains_non_public_methods?(node)
return unless all_methods_public?(node)

def_nodes(node).each do |def_node|
next unless node_visibility(def_node) == :public

message = format(MSG, preferred: "def self.#{def_node.method_name}")
add_offense(def_node, message: message) do |corrector|
extract_def_from_sclass(def_node, node, corrector)
end
add_offense(node, message: MSG_SCLASS) do |corrector|
autocorrect_sclass(node, corrector)
end
end

Expand All @@ -94,8 +91,11 @@ def def_self_style?
style == :def_self
end

def contains_non_public_methods?(sclass_node)
def_nodes(sclass_node).any? { |def_node| node_visibility(def_node) != :public }
def all_methods_public?(sclass_node)
def_nodes = def_nodes(sclass_node)
return false if def_nodes.empty?

def_nodes.all? { |def_node| node_visibility(def_node) == :public }
end

def def_nodes(sclass_node)
Expand All @@ -111,19 +111,45 @@ def def_nodes(sclass_node)
end
end

def extract_def_from_sclass(def_node, sclass_node, corrector)
def autocorrect_sclass(node, corrector)
rewritten_defs = []

def_nodes(node).each do |def_node|
next unless node_visibility(def_node) == :public

range, source = extract_def_from_sclass(def_node, node)

corrector.remove(range)
rewritten_defs << source
end

if sclass_only_has_methods?(node)
corrector.remove(node)
rewritten_defs.first&.strip!
else
corrector.insert_after(node, "\n")
end

corrector.insert_after(node, rewritten_defs.join("\n"))
end

def sclass_only_has_methods?(node)
node.body.def_type? || node.body.each_child_node.all?(&:def_type?)
end

def extract_def_from_sclass(def_node, sclass_node)
range = source_range_with_comment(def_node)
source = range.source.sub!(
"def #{def_node.method_name}",
"def self.#{def_node.method_name}"
)

corrector.insert_before(sclass_node, "#{source}\n#{indent(sclass_node)}")
corrector.remove(range)
source = source.gsub(/^ {#{indentation_diff(def_node, sclass_node)}}/, '')
[range, source.chomp]
end

def indent(node)
' ' * node.loc.column
def indentation_diff(node1, node2)
node1.loc.column - node2.loc.column
end
end
end
Expand Down
132 changes: 119 additions & 13 deletions spec/rubocop/cop/style/class_methods_definitions_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,23 @@
expect_offense(<<~RUBY)
class A
class << self
^^^^^^^^^^^^^ Do not define public methods within class << self.
attr_reader :two
def three
^^^^^^^^^ Use `def self.three` to define class method.
end
end
end
RUBY

expect_correction(<<~RUBY)
class A
#{trailing_whitespace}
def self.three
end
class << self
attr_reader :two
end
def self.three
end
end
RUBY
end
Expand All @@ -36,27 +35,113 @@ class << self
expect_offense(<<~RUBY)
class A
class << self
^^^^^^^^^^^^^ Do not define public methods within class << self.
attr_reader :one
# Multiline
# comment.
def two
^^^^^^^ Use `def self.two` to define class method.
end
end
end
RUBY

expect_correction(<<~RUBY)
class A
#{trailing_whitespace}
# Multiline
# comment.
def self.two
class << self
attr_reader :one
end
# Multiline
# comment.
def self.two
end
end
RUBY
end

it 'correctly handles class << self containing multiple methods' do
expect_offense(<<~RUBY)
class A
class << self
^^^^^^^^^^^^^ Do not define public methods within class << self.
def one
:one
end
def two
:two
end
end
end
RUBY

expect_correction(<<~RUBY)
class A
def self.one
:one
end
def self.two
:two
end
end
RUBY
end

it 'removes empty class << self when correcting' do
expect_offense(<<~RUBY)
class A
def self.one
end
class << self
attr_reader :one
^^^^^^^^^^^^^ Do not define public methods within class << self.
def two
end
def three
end
end
end
RUBY

expect_correction(<<~RUBY)
class A
def self.one
end
def self.two
end
def self.three
end
end
RUBY
end

it 'correctly handles def self.x within class << self' do
expect_offense(<<~RUBY)
class A
class << self
^^^^^^^^^^^^^ Do not define public methods within class << self.
def self.one
end
def two
end
end
end
RUBY

expect_correction(<<~RUBY)
class A
class << self
def self.one
end
end
def self.two
end
end
RUBY
Expand All @@ -71,13 +156,23 @@ def one
private
def one
def two
end
end
end
RUBY
end

it 'does not register an offense when class << self does not contain methods' do
expect_no_offenses(<<~RUBY)
class A
class << self
attr_reader :one
end
end
RUBY
end

it 'does not register an offense when defining class methods with `def self.method`' do
expect_no_offenses(<<~RUBY)
class A
Expand All @@ -97,6 +192,17 @@ def one
end
RUBY
end

it 'does not register an offense when class << self contains only class methods' do
expect_no_offenses(<<~RUBY)
class A
class << self
def self.one
end
end
end
RUBY
end
end

context 'when EnforcedStyle is self_class' do
Expand All @@ -108,7 +214,7 @@ def one
expect_offense(<<~RUBY)
class A
def self.one
^^^^^^^^^^^^ Use `class << self` to define class method.
^^^^^^^^^^^^ Use `class << self` to define a class method.
end
end
RUBY
Expand Down

0 comments on commit 295b9a6

Please sign in to comment.