Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for variable alignment to Layout/RescueEnsureAlignment #7531

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,7 @@
* [#8164](https://github.com/rubocop-hq/rubocop/pull/8164): Support auto-correction for `Lint/InterpolationCheck`. ([@koic][])
* [#8223](https://github.com/rubocop-hq/rubocop/pull/8223): Support auto-correction for `Style/IfUnlessModifierOfIfUnless`. ([@koic][])
* [#8172](https://github.com/rubocop-hq/rubocop/pull/8172): Support auto-correction for `Lint/SafeNavigationWithEmpty`. ([@koic][])
* [#7531](https://github.com/rubocop-hq/rubocop/pull/7531): Add support for variable alignment to Layout/RescueEnsureAlignment. ([@dylanahsmith][])

### Bug fixes

Expand Down
4 changes: 4 additions & 0 deletions config/default.yml
Expand Up @@ -1059,6 +1059,10 @@ Layout/ParameterAlignment:

Layout/RescueEnsureAlignment:
Description: 'Align rescues and ensures correctly.'
EnforcedStyleAlignWith: keyword
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply EnforcedStyle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked like the alignment cops were using EnforcedStyleAlignWith, so I was just trying to be consistent with them. Why not simply EnforcedStyle for Layout/BlockAlignment, Layout/DefEndAlignment, Layout/EndAlignment and Layout/RescueEnsureAlignment? I'm fine either way. Did you want me to change it to EnforcedStyle?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked like the alignment cops were using EnforcedStyleAlignWith

That is my experience/expectation as well, FWIW

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I no longer remember why we did it like this in the past, but I definitely don't like it today. Probably it was to avoid styles named align_with_*, but I think that's fine.

SupportedStylesAlignWith:
- keyword
- variable
Enabled: true
VersionAdded: '0.49'

Expand Down
44 changes: 42 additions & 2 deletions docs/modules/ROOT/pages/cops_layout.adoc
Expand Up @@ -4757,25 +4757,65 @@ end
This cop checks whether the rescue and ensure keywords are aligned
properly.

Two modes are supported through the `EnforcedStyleAlignWith`
configuration parameter:

If it's set to `keyword` (which is the default), the `rescue`
shall be aligned with the start of the begin keyword.

If it's set to `variable` the `rescue` shall be aligned with the
left-hand-side of the variable assignment, if there is one.

=== Examples

==== EnforcedStyleAlignWith: keyword (default)

[source,ruby]
----
# bad
begin
result = begin
something
rescue
puts 'error'
end

# good
result = begin
something
rescue
puts 'error'
end
----

==== EnforcedStyleAlignWith: variable

[source,ruby]
----
# bad
result = begin
something
rescue
puts 'error'
end

# good
begin
result = begin
something
rescue
puts 'error'
end
----

=== Configurable attributes

|===
| Name | Default value | Configurable values

| EnforcedStyleAlignWith
| `keyword`
| `keyword`, `variable`
|===

== Layout/SpaceAfterColon

|===
Expand Down
40 changes: 35 additions & 5 deletions lib/rubocop/cop/layout/rescue_ensure_alignment.rb
Expand Up @@ -6,22 +6,48 @@ module Layout
# This cop checks whether the rescue and ensure keywords are aligned
# properly.
#
# @example
# Two modes are supported through the `EnforcedStyleAlignWith`
# configuration parameter:
#
# If it's set to `keyword` (which is the default), the `rescue`
# shall be aligned with the start of the begin keyword.
#
# If it's set to `variable` the `rescue` shall be aligned with the
# left-hand-side of the variable assignment, if there is one.
#
# @example EnforcedStyleAlignWith: keyword (default)
#
# # bad
# result = begin
# something
# rescue
# puts 'error'
# end
#
# # good
# result = begin
# something
# rescue
# puts 'error'
# end
#
# @example EnforcedStyleAlignWith: variable
#
# # bad
# begin
# result = begin
# something
# rescue
# puts 'error'
# end
#
# # good
# begin
# result = begin
# something
# rescue
# puts 'error'
# end
class RescueEnsureAlignment < Cop
include ConfigurableEnforcedStyle
include RangeHelp

MSG = '`%<kw_loc>s` at %<kw_loc_line>d, %<kw_loc_column>d is not ' \
Expand All @@ -41,6 +67,10 @@ def on_ensure(node)
check(node)
end

def style_parameter_name
'EnforcedStyleAlignWith'
end

def autocorrect(node)
whitespace = whitespace_range(node)
# Some inline node is sitting before current node.
Expand Down Expand Up @@ -122,8 +152,8 @@ def alignment_source(node, starting_loc)
def alignment_node(node)
ancestor_node = ancestor_node(node)

return ancestor_node if ancestor_node.nil? ||
ancestor_node.kwbegin_type?
return ancestor_node if ancestor_node.nil?
return ancestor_node if style == :keyword && ancestor_node.kwbegin_type?

assignment_node = assignment_node(ancestor_node)
return assignment_node if same_line?(ancestor_node, assignment_node)
Expand Down
132 changes: 83 additions & 49 deletions spec/rubocop/cop/layout/rescue_ensure_alignment_spec.rb
@@ -1,62 +1,106 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Layout::RescueEnsureAlignment, :config do
let(:cop_config) do
{ 'EnforcedStyleAlignWith' => 'keyword', 'AutoCorrect' => true }
end

it 'accepts the modifier form' do
expect_no_offenses('test rescue nil')
end

context 'rescue with begin' do
it 'registers an offense' do
expect_offense(<<~RUBY)
begin
something
rescue
^^^^^^ `rescue` at 3, 4 is not aligned with `begin` at 1, 0.
error
end
it 'rescue not aligned with begin registers an offense' do
expect_offense(<<~RUBY)
begin
something
rescue
^^^^^^ `rescue` at 3, 4 is not aligned with `begin` at 1, 0.
error
end
RUBY

expect_correction(<<~RUBY)
begin
something
rescue
error
end
RUBY
end

context 'on assignment with keyword style' do
let(:cop_config) do
{ 'EnforcedStyleAlignWith' => 'keyword', 'AutoCorrect' => true }
end

it 'accepts multi-line, aligned with keyword' do
expect_no_offenses(<<~RUBY)
x ||= begin
1
rescue
2
end
RUBY
end

expect_correction(<<~RUBY)
begin
something
it 'accepts multi-line, indented' do
expect_no_offenses(<<~RUBY)
x ||=
begin
1
rescue
2
end
RUBY
end

it 'registers offense for incorrect alignment' do
expect_offense(<<~RUBY)
x ||= begin
1
rescue
error
^^^^^^ `rescue` at 3, 0 is not aligned with `begin` at 1, 6.
2
end
RUBY
end
end

context 'as RHS of assignment' do
it 'accepts multi-line, aligned' do
expect_no_offenses(<<~RUBY)
x ||= begin
1
rescue
2
end
RUBY
end
context 'on assignment with variable style' do
let(:cop_config) do
{ 'EnforcedStyleAlignWith' => 'variable', 'AutoCorrect' => true }
end

it 'accepts multi-line, indented' do
expect_no_offenses(<<~RUBY)
x ||=
begin
1
rescue
2
end
RUBY
end
it 'accepts multi-line, aligned with variable' do
expect_no_offenses(<<~RUBY)
x ||= begin
1
rescue
2
end
RUBY
end

it 'registers offense for incorrect alignment' do
expect_offense(<<~RUBY)
x ||= begin
it 'registers offense for multi-line, indented' do
expect_no_offenses(<<~RUBY)
x ||=
begin
1
rescue
^^^^^^ `rescue` at 3, 0 is not aligned with `begin` at 1, 6.
2
end
RUBY
end
RUBY
end

it 'registers offense for keyword alignment' do
expect_offense(<<~RUBY)
x ||= begin
1
rescue
^^^^^^ `rescue` at 3, 6 is not aligned with `x` at 1, 0.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

2
end
RUBY
end
end

Expand Down Expand Up @@ -286,16 +330,6 @@ def method2
RUBY
end

it 'accepts correctly aligned rescue in assigned begin-end block' do
expect_no_offenses(<<-RUBY)
foo = begin
bar
rescue BazError
qux
end
RUBY
end

context '>= Ruby 2.5', :ruby25 do
it 'accepts aligned rescue in do-end block' do
expect_no_offenses(<<~RUBY)
Expand Down