Navigation Menu

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

Make some cops aware of safe navigation operator #6749

Merged

Conversation

hoshinotsuyoshi
Copy link
Contributor

@hoshinotsuyoshi hoshinotsuyoshi commented Feb 9, 2019

Problem

Most of the cops do not detect &.(safe navigation operator) method offence.

For example, Style/BracesAroundHashParameters does not aware of code that uses &..
:

foo.test({ a: :b }) 
# => Adds an offence(Style/BracesAroundHashParameters).

foo&.test({ a: :b })
# => No offence. :(

(Tested by rubocop v0.62.0.)

This PR will solve some kind of this problem.

Similar PR in past

Listing target cops

I listed target cops from two perspectives:

  1. Having #on_send but not having #on_csend.
  2. Meaningful to implement(as far as I thought ).

Target cops 👮👮👮

Layout/AlignParameters c7fd520
Layout/ClosingParenthesisIndentation 146e3bf
Layout/DotPosition 9d99087
Layout/EmptyLinesAroundArguments 826cfed
Layout/FirstMethodArgumentLineBreak e8deefb
Layout/FirstParameterIndentation e769ca6
Layout/IndentArray 5783565
Layout/IndentationWidth de1dc7f
Layout/IndentHash d7b56b8
Layout/SpaceBeforeFirstArg 3ae1e4e
Lint/AmbiguousBlockAssociation 9bcd5c8
Lint/EachWithObjectArgument 8e1945f
Lint/ParenthesesAsGroupedExpression bedcc22
Lint/RequireParentheses fa699cb
Rails/ActiveRecordAliases b35a848
Rails/Date 15da862
Rails/DynamicFindBy 55fc1a5
Rails/FindBy dadab3d
Rails/SaveBang c1ec1d1
Rails/SkipsModelValidations a3ad8b6
Style/BracesAroundHashParameters d959982
Style/MethodCalledOnDoEndBlock 995f4c1
Style/MethodCallWithArgsParentheses dcb3b34
Style/NestedParenthesizedCalls a285622
Style/PreferredHashMethods 5fc114c
Style/Send 4f6c20b
Style/StringMethods 82402bc
Style/TrailingCommaInArguments fcd785c

TODO

  • Squash commits if ok

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.

@@ -31,7 +31,7 @@ def extract_rhs(node)
_scope, _lhs, rhs = *node
elsif node.op_asgn_type?
_lhs, _op, rhs = *node
elsif node.send_type?
elsif node.send_type? || node.csend_type?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case for this change?
RSpec is still green if I remove this change.

I guess it supports something.some_method = 1 style assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RSpec is still green if I remove this change.

Sorry, and thank you!

Pushed 48415e2

@pocke
Copy link
Collaborator

pocke commented Feb 9, 2019

Great work! Thiank you for your pull request! 👍 👍

@pocke pocke merged commit 548bf43 into rubocop:master Feb 10, 2019
@koic
Copy link
Member

koic commented Feb 10, 2019

My review was delayed.
IMO, I was glad if this PR squashed these commits into one. It is because I wanted to export one commit reason for one change with one cherry-pick when exporting to RuboCop Rails as a real work :-), but thanks anyway!

@Drenmi Drenmi mentioned this pull request Feb 10, 2019
@hoshinotsuyoshi
Copy link
Contributor Author

😱 I'm sorry, I intentionally pushed such multiple commits to make it easier to read for review. 🙇

IMO, I was glad if this PR squashed these commits into one.

I knew how to do it and was going to do so.
I should have done so from the beginning!

I will do so next time.

Anyway, thank you !! :)

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