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 #6755] Prevent Style/TrailingCommaInArgument from breaking when a safe method call is chained on the offending method #6757

Merged

Conversation

Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Feb 11, 2019

This cop would error out on code like:

foo.bar(
  baz: 1,
)&.fetch(:qux)

This was happening because the cop wasn't taking csend (safe navigation) node types into consideration.


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.

@hoshinotsuyoshi
Copy link
Contributor

@Drenmi
🙇 Sorry, It seems that PR #6749 caused the error. And thank you for your quick responding!

🙇 I found another point in #method_name_and_arguments_on_same_line? and test case to fix...

patch: hoshinotsuyoshi@f25ee5b

diff --git a/lib/rubocop/cop/mixin/trailing_comma.rb b/lib/rubocop/cop/mixin/trailing_comma.rb
index f9754b07c..d2d91e1a8 100644
--- a/lib/rubocop/cop/mixin/trailing_comma.rb
+++ b/lib/rubocop/cop/mixin/trailing_comma.rb
@@ -91,7 +91,7 @@ module RuboCop
       end

       def method_name_and_arguments_on_same_line?(node)
-        node.send_type? &&
+        %i[csend send].include?(node.type) &&
           node.loc.selector.line == node.arguments.last.last_line &&
           node.last_line == node.arguments.last.last_line
       end
diff --git a/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb b/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb
index 77e0b3077..8ef6627ba 100644
--- a/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb
+++ b/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb
@@ -39,6 +39,14 @@ RSpec.describe RuboCop::Cop::Style::TrailingCommaInArguments, :config do
       RUBY
     end

+    it 'accepts method call without trailing comma ' \
+       'when a line break before a method call', :ruby23 do
+      expect_no_offenses(<<-RUBY.strip_indent)
+        obj
+          &.do_something(:foo, :bar)
+      RUBY
+    end
+
     it 'accepts method call without trailing comma with single element hash' \
         ' parameters at the end' do
       expect_no_offenses('some_method(a: 1)')

If you think it is ok, use this patch or add some code like that.

@Drenmi Drenmi force-pushed the bugfix/trailing-comma-in-arguments-cop branch from 00f5c5c to 4415cd6 Compare February 12, 2019 03:17
@Drenmi
Copy link
Collaborator Author

Drenmi commented Feb 12, 2019

Thanks for following up @hoshinotsuyoshi! I have integrated your patch in the bugfix. 🙇

@Drenmi Drenmi force-pushed the bugfix/trailing-comma-in-arguments-cop branch from 4415cd6 to ede5c66 Compare February 17, 2019 10:05
@koic
Copy link
Member

koic commented Feb 17, 2019

Looks good to me. Could you please resolve the conflict?

…g when a safe method call is chained on the offending method

This cop would error out on code like:

```
foo.bar(
  baz: 1,
)&.fetch(:qux)
```

This was happening because the cop wasn't taking `csend` (safe navigation)
node types into consideration.
@Drenmi Drenmi force-pushed the bugfix/trailing-comma-in-arguments-cop branch from ede5c66 to e81e7a2 Compare February 19, 2019 02:00
@Drenmi Drenmi merged commit 8fe4946 into rubocop:master Feb 19, 2019
@Drenmi Drenmi deleted the bugfix/trailing-comma-in-arguments-cop branch February 19, 2019 02:14
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

3 participants