Skip to content

Add new cop Style/EndlessMethod.#9281

Merged
bbatsov merged 1 commit intorubocop:masterfrom
dvandersluis:style/endless-methods
Jan 3, 2021
Merged

Add new cop Style/EndlessMethod.#9281
bbatsov merged 1 commit intorubocop:masterfrom
dvandersluis:style/endless-methods

Conversation

@dvandersluis
Copy link
Copy Markdown
Member

@dvandersluis dvandersluis commented Dec 22, 2020

Style/EndlessMethod by default enforces that endless methods definitions must be on a single line (EnforcedStyle: allow). It can also be configured to disallow endless method definitions (EnforcedStyle: disallow), or to allow every endless method regardless of lines (EnforcedStyle: allow_always).

This cop (obviously) only applies if TargetRubyVersion is at least 3.0.

There is a style guide issue open for this: rubocop/ruby-style-guide#858.


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.

@dvandersluis
Copy link
Copy Markdown
Member Author

The documentation check failure is already fixed in #9276 (I can cherry-pick the fix commit into here as well, or once that PR is merged I'll rebase).

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

The cop looks solid overall, but I'm wondering if this should be a standalone cop or some configuration of the SingleLineMethods cop. As endless methods are always single-line it makes sense to me to just fold them under the existing cop and let the users decide if they want to stick to classic approach or endless methods via different enforced styles.

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

After getting this feedback from @mame I'm thinking the cop should actually be based on it. We can have two supported styles:

  • allow the new syntax in the cases outlined by @mame (default)
  • allow it always
  • forbid it always

In light of this, you can forget about my suggestion to merge this cop with SingleLineMethod. I didn't think of the case when you can write a multi-line endless method.

@dvandersluis
Copy link
Copy Markdown
Member Author

Just to avoid the eventual bikeshed 😅 what are we going to define the allowed default case as? (I probably won't get to this for at least a couple days but I might have time who knows).

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 24, 2020

I was thinking by default we can allow only single-line endless methods and refine the definition later to potentially account for definition simplicity and side-effects (e.g. based on configurable list of side-effect producing methods).

@dvandersluis
Copy link
Copy Markdown
Member Author

Totally agree. I’ll make it happen.

@dvandersluis
Copy link
Copy Markdown
Member Author

@bbatsov I've redone this cop based on the above comments. I removed the changes to SingleLineMethod, but I'm going to open a PR for that too to allow single line methods to be rewritten to endless methods.

The styles I ended up with are allow (only single line), allow_always (any line length), and disallow. I used allow to represent the above discussion (since we might change how it applies based on feedback), but if we don't want to change it, allow_single_line might be a better name.

@koic
Copy link
Copy Markdown
Member

koic commented Dec 28, 2020

allow_single_line might be a better name.

I like this better name. I think that allow_single_line is clearer and good than allow.

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 30, 2020

Yeah, I agree. Other than this, the cop seems solid to me.

@dvandersluis
Copy link
Copy Markdown
Member Author

@bbatsov @koic updated to allow_single_line.

Comment thread config/default.yml Outdated
@bbatsov bbatsov merged commit 41928e8 into rubocop:master Jan 3, 2021
@dvandersluis dvandersluis deleted the style/endless-methods branch January 18, 2021 20:43
koic added a commit to koic/rubocop that referenced this pull request Jan 26, 2021
Follow rubocop#9281 (comment).

This PR fixes a typo for `Style/SingleLineMethods`.

`EnforcedStyle: allow` -> `EnforcedStyle: allow_single_line`
@koic koic mentioned this pull request Jan 26, 2021
8 tasks
bbatsov pushed a commit that referenced this pull request Jan 26, 2021
Follow #9281 (comment).

This PR fixes a typo for `Style/SingleLineMethods`.

`EnforcedStyle: allow` -> `EnforcedStyle: allow_single_line`
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.

3 participants