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

Layout/MultilineAssignmentLayout causes weird indentation #8582

Closed
ioquatix opened this issue Aug 25, 2020 · 3 comments · Fixed by #8628
Closed

Layout/MultilineAssignmentLayout causes weird indentation #8582

ioquatix opened this issue Aug 25, 2020 · 3 comments · Fixed by #8628

Comments

@ioquatix
Copy link

Here is the source code test case:

# Is it broken?
def my_method
  return [] if my_thing
  foo = if condition
          foo = bar rescue "{}"
          update(foo)
          JSON.parse(foo) rescue []
        else
          JSON.parse(stuff) rescue []
        end
  questions
end

Here is the configuration:

Layout/MultilineAssignmentLayout:
  Enabled: true

Layout/EndAlignment:
  Enabled: true
  EnforcedStyleAlignWith: variable
  AutoCorrect: true

If you disable Layout/MultilineAssignmentLayout it works better.

Expected behavior

# Is it broken?
def my_method
  return [] if my_thing

  foo =
    if condition
      foo =
        begin
          bar
        rescue StandardError
          '{}'
        end
      update(foo)
      begin
        JSON.parse(foo)
      rescue StandardError
        []
      end
    else
      begin
        JSON.parse(stuff)
      rescue StandardError
        []
      end
    end
  questions
end

Actual behavior

# Is it broken?
def my_method
  return [] if my_thing

  foo =
    if condition
      foo =
        begin
                           bar
        rescue StandardError
          '{}'
                         end
      update(foo)
      begin
        JSON.parse(foo)
      rescue StandardError
        []
      end
    else
      begin
        JSON.parse(stuff)
      rescue StandardError
        []
      end
    end
  questions
end

Steps to reproduce the problem

rubocop -a

RuboCop version

koyoko% rubocop -V
0.89.1 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.7.1 x86_64-linux)
@ioquatix
Copy link
Author

Actually, I'm wrong, it's not Layout/MultilineAssignmentLayout causing the issue, but maybe Style/RescueModifier.

koic added a commit to koic/rubocop that referenced this issue Sep 6, 2020
Fixes rubocop#8582.

This PR adds `Layout/BeginEndAlignment` cop to solve the issue.

The following is an example:

```ruby
% cat example.rb
foo = bar rescue "{}"
```

## Before

The alignment was misaligned because `begin` was not awake.

```ruby
% rubocop -a example.rb
(snip)

% cat example.rb
foo =
  begin
         bar
  rescue StandardError
    '{}'
       end
```

## After

Awakened by `begin`, so aligned.

```ruby
% rubocop -a example.rb
(snip)

% cat example.rb
foo =
  begin
    bar
  rescue StandardError
    '{}'
  end
```

I was first planning to add `begin` processing to `Layout/EndAlignment` cop.
However, `Layout/EndAlignment` cop with `EnforcedStyleAlignWith: keyword (default)`
was not preferred code in the following:

```ruby
longlonglonglonglonglonglonglonglong ||= begin
                                           do_something
                                         end
```

This style has been found to have very long line length with RuboCop's own code.

Therefore, since this PR would like to use `EnforcedStyleAlignWith: start_of_line`
by default against `foo ||= begin`, this PR added the new cop.

```ruby
# EnforcedStyleAlignWith: start_of_line (default)
longlonglonglonglonglonglonglonglong ||= begin
  do_something
end
```

This solution allows configuration with minimal impact on existing code.

It is configurable if user want to align to `begin`.

```ruby
# EnforcedStyleAlignWith: begin
longlonglonglonglonglonglonglonglong ||= begin
                                           do_something
                                         end
```

This PR also makes `Layout/RescueEnsureAlignment` cop aware of new
`Layout/BeginEndAlignment` cop's config to align `rescue` indents.
@koic koic closed this as completed in #8628 Sep 6, 2020
koic added a commit that referenced this issue Sep 6, 2020
…_cop

[Fix #8582] Add new `Layout/BeginEndAlignment` cop
@ioquatix
Copy link
Author

ioquatix commented Sep 6, 2020

Thanks!

@koic
Copy link
Member

koic commented Sep 6, 2020

@ioquatix You are welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants