Break long method definitions when auto-correcting #10692

Korri · 2022-06-03T21:13:44Z

Add autocorrect capabilities to Layout/LineLength, by wrapping method definitions on multiple lines.

# bad
def foo(abc: "100000", def: "100000", ghi: "100000", jkl: "100000", mno: "100000")
end

# good (autocorrect)
def foo(abc: "100000", def: "100000", 
  ghi: "100000", jkl: "100000", mno: "100000")
end

Combined with #10691 it allows for some nice autocorrect.

This was split from the larger WIP pull request here: #10681

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Korri · 2022-06-03T21:19:57Z

lib/rubocop/cop/mixin/check_line_breakable.rb

@@ -216,6 +218,8 @@ def process_args(args)

      # @api private
      def already_on_multiple_lines?(node)
+        return node.first_line != node.arguments.last.last_line if node.def_type?


This is needed as node.last_line returns the line where the end keyword is. Maybe there is a better approach here, the downside of the approach I chose is that:

def foo(bar, bat ) end

Does get wrapped on multiple lines. That's not strictly a bad behaviour as this only triggers when the line is longer then the maximum length, but still somewhat unexpected.

nobuyo · 2022-06-04T03:55:25Z

I've run this feature locally and infinite loop is detected on 98981457abc38c3edd1f6a2a738f144314fedd0a.

# .rubocop.yml
AllCops:
  NewCops: enable
  TargetRubyVersion: 3.1

Layout/LineLength:
  Max: 40

# example.rb
def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000'); end

$ bundle exec rubocop -a example.rb
Inspecting 1 file
W

Offenses:

example.rb:1:1: C: [Corrected] Style/EmptyMethod: Put empty method definitions on a single line.
def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000'); ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
example.rb:1:38: C: [Corrected] Layout/TrailingWhitespace: Trailing whitespace detected.
def foo(abc: '100000', def: '100000',
                                     ^
example.rb:1:41: C: [Corrected] Layout/LineLength: Line is too long. [83/40]
def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000');
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
example.rb:1:41: C: [Corrected] Layout/LineLength: Line is too long. [87/40]
def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000'); end
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
example.rb:1:83: C: [Corrected] Style/Semicolon: Do not use semicolons to terminate expressions.
def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000');
                                                                                  ^
example.rb:2:1: C: [Corrected] Layout/ParameterAlignment: Align the parameters of a method definition if they span more than one line.
ghi: '100000', jkl: '100000', mno: '100000')
^^^^^^^^^^^^^
example.rb:2:2: W: [Corrected] Layout/DefEndAlignment: end at 2, 1 is not aligned with def at 1, 0.
 end
 ^^^
example.rb:2:41: C: Layout/LineLength: Line is too long. [52/40]
        ghi: '100000', jkl: '100000', mno: '100000')
                                        ^^^^^^^^^^^^

0 files inspected, 8 offenses detected, 7 offenses corrected
Infinite loop detected in /Users/nobuyo/dev/rubocop-test/example.rb and caused by Layout/LineLength -> Layout/DefEndAlignment, Layout/LineLength, Style/EmptyMethod, Style/Semicolon -> Layout/ParameterAlignment, Layout/TrailingWhitespace, Style/EmptyMethod -> Layout/LineLength, Style/EmptyMethod
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:296:in `check_for_infinite_loop'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:279:in `block in iterate_until_no_changes'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:278:in `loop'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:278:in `iterate_until_no_changes'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:247:in `do_inspection_loop'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:130:in `block in file_offenses'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:155:in `file_offense_cache'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:129:in `file_offenses'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:120:in `process_file'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:101:in `block in each_inspected_file'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:100:in `each'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:100:in `reduce'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:100:in `each_inspected_file'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:86:in `inspect_files'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/runner.rb:47:in `run'
/Users/nobuyo/dev/rubocop-korri/lib/rubocop/cli/command/execute_runner.rb:26:in `block in execute_runner'

# ...

Korri · 2022-06-07T13:46:49Z

I've run this feature locally and infinite loop is detected on 9898145.

@nobuyo Looks like you example is already an infinite loop on the master branch, Style/EmptyMethod (with EnforcedStyle: compact) doesn't play well with Layout/LineLength.

Using the expanded style does work:

# .rubocop.yml
AllCops:
  NewCops: enable
  TargetRubyVersion: 3.1

Layout/LineLength:
  Max: 40
Style/EmptyMethod:
  EnforcedStyle: expanded

# example.rb (after autocorrect)
def foo(abc: '100000', def: '100000',
        ghi: '100000', jkl: '100000', mno: '100000')
end

nobuyo · 2022-06-07T13:57:44Z

Oh I see, thank you!

AlexWayfer · 2022-06-15T17:21:48Z

Sorry if not relevant, this PR is pretty short and laconic, just like a warning:

I like configuration like this:

Layout/FirstArgumentIndentation:
  EnforcedStyle: consistent
Layout/FirstParameterIndentation:
  EnforcedStyle: consistent
Layout/FirstArrayElementIndentation:
  EnforcedStyle: consistent
Layout/FirstHashElementIndentation:
  EnforcedStyle: consistent
Layout/MultilineMethodCallIndentation:
  EnforcedStyle: indented
Layout/MultilineOperationIndentation:
  EnforcedStyle: indented
Layout/MultilineArrayBraceLayout:
  EnforcedStyle: new_line
Layout/MultilineHashBraceLayout:
  EnforcedStyle: new_line
Layout/ArgumentAlignment:
  EnforcedStyle: with_fixed_indentation
Layout/ParameterAlignment:
  EnforcedStyle: with_fixed_indentation

And I'd like to see correct… style of breaking, I mean new lines instead of alignment.

I hope this is already works correctly, maybe just we should add more specs for it.

Korri · 2022-06-15T17:57:08Z

Sorry if not relevant, this PR is pretty short and laconic, just like a warning:

I like configuration like this:

Layout/FirstArgumentIndentation:
  EnforcedStyle: consistent
Layout/FirstParameterIndentation:
  EnforcedStyle: consistent
Layout/FirstArrayElementIndentation:
  EnforcedStyle: consistent
Layout/FirstHashElementIndentation:
  EnforcedStyle: consistent
Layout/MultilineMethodCallIndentation:
  EnforcedStyle: indented
Layout/MultilineOperationIndentation:
  EnforcedStyle: indented
Layout/MultilineArrayBraceLayout:
  EnforcedStyle: new_line
Layout/MultilineHashBraceLayout:
  EnforcedStyle: new_line
Layout/ArgumentAlignment:
  EnforcedStyle: with_fixed_indentation
Layout/ParameterAlignment:
  EnforcedStyle: with_fixed_indentation

And I'd like to see correct… style of breaking, I mean new lines instead of alignment.

I hope this is already works correctly, maybe just we should add more specs for it.

Not sure what you are concerned about (as in not sure I understand)? The change here is so that LineLength can wrap the method definition on it's own.
Once the method definition is wrapped it becomes "multiline" and other cops can take over, including the new Layout/MultilineMethodParameterLineBreaks I'm proposing to introduce here: #10691

AlexWayfer · 2022-06-15T18:16:39Z

Not sure what you are concerned about (as in not sure I understand)? The change here is so that LineLength can wrap the method definition on it's own.

I mean you have specs like this:

expect_correction(<<~RUBY)
  def foo(abc: "100000", def: "100000",#{trailing_whitespace}
  ghi: "100000", jkl: "100000", mno: "100000")
  end
RUBY

And probably there should be additional context with another style of FirstParameterIndentation and different expectation. Just to be sure that it works. align_parentheses, as I see, so it'd be (I believe):

expect_correction(<<~RUBY)
  def foo(
          abc: "100000", def: "100000",#{trailing_whitespace}
ghi: "100000", jkl: "100000", mno: "100000")
  end
RUBY

Once the method definition is wrapped it becomes "multiline" and other cops can take over, including the new Layout/MultilineMethodParameterLineBreaks I'm proposing to introduce here: #10691

I'm discussing this small PR, and description and specs what I've seen. I'm not sure about (draft and large) 10691, maybe there it's resolved, but, again, I'm talking just about this PR and maybe I'm wrong. So, that's why I wrote from the start: "may be non-relevant and it's just a warning".

I hope it's more understandable now.

Korri · 2022-06-15T19:07:11Z

@AlexWayfer I think what you are describing is Layout/FirstMethodParameterLineBreak here is an example of how those rule combine to nicelly autocorrect:

# .rubocop.yml
AllCops:
  NewCops: enable
  TargetRubyVersion: 3.1

Layout/LineLength:
  Max: 40
Style/EmptyMethod:
  EnforcedStyle: expanded
Layout/FirstParameterIndentation:
  EnforcedStyle: consistent
Layout/ParameterAlignment:
  EnforcedStyle: with_fixed_indentation
Layout/FirstMethodParameterLineBreak:
  Enabled: true

# source (bad)
# frozen_string_literal: true

def foo(abc: '100000', def: '100000', ghi: '100000', jkl: '100000', mno: '100000')
end

# result (good)
# frozen_string_literal: true

def foo(
  abc: '100000', def: '100000',
  ghi: '100000', jkl: '100000', mno: '100000'
)
end

(If you run the same example on the master branch, nothing will get autocorrected)

The goal of the change here, is just for LineLength to wrap the method definition on multiple lines, so that other Cop can take over and format it the way you like.

As for specs, as far as I have seen, the codebase does not have specs for combined rules!

This allows rubocop to autocorrect method definitions by wrapping them on multiple lines when they violate the `Layout/LineLength` rule by adding method definition logic inside `CheckLineBreakable`.

Korri · 2022-07-12T20:29:33Z

@bbatsov for context Layout/MultilineMethodParameterLineBreaks won't be auto-correctable for long lines until this PR is merged too!

bbatsov · 2022-07-13T16:20:10Z

Looks good. Sorry for the slow turnaround here!

Korri force-pushed the wrap-method-definition branch 2 times, most recently from 05b0523 to 9898145 Compare June 3, 2022 21:16

Korri commented Jun 3, 2022

View reviewed changes

Korri marked this pull request as ready for review June 3, 2022 21:22

nobuyo mentioned this pull request Jun 7, 2022

Layout/LineLength and some cops causes infinite loop #10700

Closed

Break long method definitions when auto-correcting

c06ff54

This allows rubocop to autocorrect method definitions by wrapping them on multiple lines when they violate the `Layout/LineLength` rule by adding method definition logic inside `CheckLineBreakable`.

Korri force-pushed the wrap-method-definition branch from 9898145 to c06ff54 Compare July 12, 2022 20:26

bbatsov merged commit 8d484e3 into rubocop:master Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break long method definitions when auto-correcting #10692

Break long method definitions when auto-correcting #10692

Korri commented Jun 3, 2022 •

edited

Korri Jun 3, 2022

nobuyo commented Jun 4, 2022 •

edited

Korri commented Jun 7, 2022 •

edited

nobuyo commented Jun 7, 2022

AlexWayfer commented Jun 15, 2022

Korri commented Jun 15, 2022 •

edited

AlexWayfer commented Jun 15, 2022

Korri commented Jun 15, 2022

Korri commented Jul 12, 2022

bbatsov commented Jul 13, 2022

Break long method definitions when auto-correcting #10692

Break long method definitions when auto-correcting #10692

Conversation

Korri commented Jun 3, 2022 • edited

Korri Jun 3, 2022

Choose a reason for hiding this comment

nobuyo commented Jun 4, 2022 • edited

Korri commented Jun 7, 2022 • edited

nobuyo commented Jun 7, 2022

AlexWayfer commented Jun 15, 2022

Korri commented Jun 15, 2022 • edited

AlexWayfer commented Jun 15, 2022

Korri commented Jun 15, 2022

Korri commented Jul 12, 2022

bbatsov commented Jul 13, 2022

Korri commented Jun 3, 2022 •

edited

nobuyo commented Jun 4, 2022 •

edited

Korri commented Jun 7, 2022 •

edited

Korri commented Jun 15, 2022 •

edited