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

Change AllowAdjacentOneLineDefs config parameter of Layout/EmptyLineBetweenDefs to true by default #10199

Conversation

koic
Copy link
Member

@koic koic commented Oct 20, 2021

This PR changes AllowAdjacentOneLineDefs config parameter of Layout/EmptyLineBetweenDefs to true by default.

Currently, warnings that occur are different for each type of definition grouped as shown below.

% cat example.rb
class FooError < StandardError; end
class BarError < StandardError; end
class BazError < StandardError; end

attr_accessor :foo
attr_accessor :bar
attr_accessor :baz

before_action :do_foo
before_action :do_bar
before_action :do_baz
% bundle exec rubocop
(snip)

Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Correctable] Style/FrozenStringLiteralComment:
Missing frozen string literal comment.
class FooError < StandardError; end
^
example.rb:2:1: C: [Correctable] Layout/EmptyLineBetweenDefs: Expected 1
empty line between class definitions; found 0.
class BarError < StandardError; end
^^^^^^^^^^^^^^
example.rb:3:1: C: [Correctable] Layout/EmptyLineBetweenDefs: Expected 1
empty line between class definitions; found 0.
class BazError < StandardError; end
^^^^^^^^^^^^^^

1 file inspected, 3 offenses detected, 3 offenses auto-correctable

This AllowAdjacentOneLineDefs: true makes it consistent so that there are no blank lines when grouped by one liner definitions.


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.

@koic koic force-pushed the change_allow_adjacent_one_line_defs_true_by_default branch 2 times, most recently from 4dfb5db to 8e0334e Compare October 20, 2021 03:29
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 20, 2021

I find the example code confusing:

class FooError < StandardError; end
class BarError < StandardError; end
class BazError < StandardError; end

attr_accessor :foo
attr_accessor :bar
attr_accessor :baz

before_action :do_foo
before_action :do_bar
before_action :do_baz

There's just one type of definition here and the macro methods. I thought the cop applies just to class/module/def. I might be misunderstanding the problem the change aims to solve.

@koic
Copy link
Member Author

koic commented Oct 22, 2021

Oh, I'm sorry. My explanation was not good. It was the behavior when running rubocop command including other entire cops by default, not just Layout/EmptyLineBetweenDefs cop.
So, I would like to convey that it would be better for grouped one-liners case to have no blank lines.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 22, 2021

Makes sense to me in general. Is this something that requires changes/new examples to the style guide as well?

@@ -77,13 +77,20 @@ module Layout
# def b
# end
#
# @example AllowAdjacentOneLineDefs: true
# @example AllowAdjacentOneLineDefs: true (default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it'd be nice to put both good and bad examples for the option. I see now with true have only good and with false only bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no bad cases because AllowAdjacentOneLineDefs: true only increases allowed good cases.
AllowAdjacentOneLineDefs: true and AllowAdjacentOneLineDefs: false add the same good case.
In any case, I've updated it and AllowAdjacentOneLineDefs: false's example is more clearer :-)

…neBetweenDefs` to `true` by default

This PR changes `AllowAdjacentOneLineDefs` config parameter of `Layout/EmptyLineBetweenDefs` to `true` by default.

Currently, warnings that occur are different for each type of definition grouped as shown below.

```console
% cat example.rb
class FooError < StandardError; end
class BarError < StandardError; end
class BazError < StandardError; end

attr_accessor :foo
attr_accessor :bar
attr_accessor :baz

before_action :do_foo
before_action :do_bar
before_action :do_baz
```

```console
% bundle exec rubocop
(snip)

Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Correctable] Style/FrozenStringLiteralComment:
Missing frozen string literal comment.
class FooError < StandardError; end
^
example.rb:2:1: C: [Correctable] Layout/EmptyLineBetweenDefs: Expected 1
empty line between class definitions; found 0.
class BarError < StandardError; end
^^^^^^^^^^^^^^
example.rb:3:1: C: [Correctable] Layout/EmptyLineBetweenDefs: Expected 1
empty line between class definitions; found 0.
class BazError < StandardError; end
^^^^^^^^^^^^^^

1 file inspected, 3 offenses detected, 3 offenses auto-correctable
```

This `AllowAdjacentOneLineDefs: true` makes it consistent so that there are no blank lines when
grouped by one liner definitions.
@koic
Copy link
Member Author

koic commented Oct 25, 2021

Makes sense to me in general. Is this something that requires changes/new examples to the style guide as well?

Good point! I will also take a look at the guide.

@koic koic force-pushed the change_allow_adjacent_one_line_defs_true_by_default branch from 8e0334e to d0e9aa1 Compare October 25, 2021 03:02
@bbatsov bbatsov merged commit 4e78e8e into rubocop:master Nov 12, 2021
@koic koic deleted the change_allow_adjacent_one_line_defs_true_by_default branch November 12, 2021 17:28
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.

None yet

2 participants