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

7492 Add Style/TrailingCommaInBlockArgs #7637

Merged
merged 2 commits into from Mar 22, 2020

Conversation

ty-porter
Copy link
Contributor

@ty-porter ty-porter commented Jan 8, 2020

Fixes #7492. This PR adds new cop Style/TrailingCommaInBlockArgs for blocks with multiple arguments. Trailing commas are only required in blocks with one argument, else a trailing comma should be omitted.


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.

lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Jan 8, 2020

There are the following differences:

% ruby -wve "{foo: 1, bar: 2, baz: 3}.each {|key,| p key }"
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin17]
:foo
:bar
:baz
% ruby -wve "{foo: 1, bar: 2, baz: 3}.each {|key| p key }"
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin17]
[:foo, 1]
[:bar, 2]
[:baz, 3]

So, some are my thoughts.

  • I think it's Style department, not Lint department
  • The good and bad cases handled by this cop are incompatible and unsafe
  • The trailing comma is an idiom that I sometimes see for me. I think disabled by default is preferable.

lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/lint/useless_trailing_comma.rb Outdated Show resolved Hide resolved
@ty-porter
Copy link
Contributor Author

ty-porter commented Jan 8, 2020

This is a lot of feedback to take in. Let me see if I can go back and make the required changes, or if not, pass along to someone who knows better than I.

@ty-porter ty-porter force-pushed the 7492-useless-trailing-comma branch 2 times, most recently from 87804f0 to f45dd42 Compare Jan 9, 2020
@ty-porter
Copy link
Contributor Author

ty-porter commented Jan 9, 2020

@koic

  • I think it's Style department, not Lint department

I didn't change this yet, but can do so if necessary.

  • The good and bad cases handled by this cop are incompatible and unsafe

This shouldn't touch trailing commas that are required (for instance a trailing comma after a single argument), so it should preserve syntax in both cases.

  • The trailing comma is an idiom that I sometimes see for me. I think disabled by default is preferable.

I disabled by default.

config/default.yml Outdated Show resolved Hide resolved
@ty-porter ty-porter force-pushed the 7492-useless-trailing-comma branch from f45dd42 to ce03289 Compare Jan 9, 2020
@ty-porter ty-porter changed the title 7492 Add Lint/UselessTrailingComma 7492 Add Style/UselessTrailingComma Jan 9, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 12, 2020

I think we need a better name for this cop, as right now it's too generic. Unfortunately I can't think of something that's not very long like RedundantCommaInBlockParameters.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 12, 2020

This can potentially get a style guide entry as well.

@ty-porter
Copy link
Contributor Author

ty-porter commented Jan 12, 2020

I think we need a better name for this cop, as right now it's too generic. Unfortunately I can't think of something that's not very long like RedundantCommaInBlockParameters.

Just noticed as I was looking for some naming convention that a TrailingCommaInArguments cop also exists but handles redundant commas in method calls, so maybe something like TrailingCommaInBlockArgs?

@ty-porter ty-porter force-pushed the 7492-useless-trailing-comma branch from ce03289 to aea4a7b Compare Jan 14, 2020
@ty-porter ty-porter force-pushed the 7492-useless-trailing-comma branch 2 times, most recently from d1a470e to a1335b4 Compare Jan 22, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 21, 2020

Just noticed as I was looking for some naming convention that a TrailingCommaInArguments cop also exists but handles redundant commas in method calls, so maybe something like TrailingCommaInBlockArgs?

@pawptart Yeah, I think that's a good idea.

It seems also you'll have to do some rebasing looking at the list of merge conflicts. :-)

@ty-porter ty-porter force-pushed the 7492-useless-trailing-comma branch 2 times, most recently from cef08d1 to 44442c1 Compare Mar 21, 2020
@ty-porter ty-porter changed the title 7492 Add Style/UselessTrailingComma 7492 Add Style/TrailingCommaInBlockArgs Mar 21, 2020
@ty-porter
Copy link
Contributor Author

ty-porter commented Mar 22, 2020

@pawptart Yeah, I think that's a good idea.

It seems also you'll have to do some rebasing looking at the list of merge conflicts. :-)

@bbatsov

Taken care of :)

@bbatsov bbatsov merged commit 22cc72c into rubocop:master Mar 22, 2020
10 of 17 checks passed
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 22, 2020

Thanks!

Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Apr 28, 2020
Co-authored-by: Bozhidar Batsov <bozhidar@batsov.com>
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.

Cop Idea: Useless trailing comma in block arguments
7 participants