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

[Fix #8582] Add new Layout/BeginEndAlignment cop #8628

Merged
merged 1 commit into from Sep 6, 2020

Conversation

koic
Copy link
Member

@koic koic commented Sep 1, 2020

Fixes #8582.

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

The following is an example:

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

Before

The alignment was misaligned because begin was not awake.

% rubocop -a example.rb
(snip)

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

After

Awakened by begin, so aligned.

% 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:

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.

# 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.

# 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.


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 2, 2020

I'm fine with proposed solution, but I think we need to update the descriptions of BeginEndAlignment and EndAlignment to make it clear why we have two cops and what exactly are their responsibilities. Even for me the current arrangement is slightly confusing, so I can imagine it'd be even more confusing for our casual users.

@koic koic force-pushed the add_new_layout_begin_end_alignment_cop branch from cf806f8 to 1979f96 Compare September 3, 2020 10:39
@koic
Copy link
Member Author

koic commented Sep 3, 2020

Ah, exactly! I updated the docs.

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 force-pushed the add_new_layout_begin_end_alignment_cop branch from 1979f96 to 1b21b76 Compare September 6, 2020 08:12
@koic koic merged commit 1876a0d into rubocop:master Sep 6, 2020
@koic koic deleted the add_new_layout_begin_end_alignment_cop branch September 6, 2020 08:31
@mobilutz
Copy link

Awesome cop. I always thought the default begin/end alignment was strange 😉

jmkoni pushed a commit to standardrb/standard that referenced this pull request Sep 16, 2020
…en fixed

* Update Rubocop from
  [0.90](https://github.com/rubocop-hq/rubocop/releases/tag/v0.90.0)
  to
  [0.91](https://github.com/rubocop-hq/rubocop/releases/tag/v0.91.0),
  enabling:
  * [`Lint/UselessTimes`](rubocop/rubocop#8702)
  * [`Layout/BeginEndAlignment`](rubocop/rubocop#8628)
  * [`Lint/ConstantDefinitionInBlock`](rubocop/rubocop#8707)
  * [`Lint/IdentityComparison`](rubocop/rubocop#8699)
  re-enabling after bug fixes:
  * [`Bundler/DuplicatedGem`](rubocop/rubocop#8666)
  * [`Naming/BinaryOperatorParameterName`](rubocop/rubocop#8664)
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 this pull request may close these issues.

Layout/MultilineAssignmentLayout causes weird indentation
3 participants