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 #12271] Fix a false positive for Lint/RedundantSafeNavigation #12272

Conversation

koic
Copy link
Member

@koic koic commented Oct 13, 2023

Fixes #12271.

This PR fixes a false positive for Lint/RedundantSafeNavigation when using snake case constant receiver.

Camel case is used for naming classes and modules, while snake case is used for all other constants. This naming conforms with Naming/ConstantName cop.

Since constants that turn into classes or modules are normally not nil, they will continue to be detected.
However, this PR will update to allow safe navigation for constants in snake case.

This change resolves both issue rubocop/rubocop-rails#1104 and #12271.


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 fix_a_false_positive_for_lint_redundant_safe_navigation branch from f3bd8b8 to 864c14e Compare October 13, 2023 04:20
# Use cases where a constant is `nil` are rare and an offense is detected
# when the receiver is a constant.
# Use cases where a constant, named in camel case for classes and modules is `nil` are rare,
# and an offense is detected when the receiver is a snake case constant.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you meant to write "is not detected" here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you mean to target just SCREAMING_SNAKE_CASE. I see.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! I've updated the text.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 13, 2023

Frankly, I'm not sure what's the best course of action here, as I don't understand why anyone would have a nil constant regardless of whether it's a class/module reference or not.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 13, 2023

P.S. Probably we need some cop about assigning nil to constants, as it makes very little sense to me.

@koic koic force-pushed the fix_a_false_positive_for_lint_redundant_safe_navigation branch from 864c14e to a974a5a Compare October 13, 2023 05:52
@koic
Copy link
Member Author

koic commented Oct 13, 2023

P.S. Probably we need some cop about assigning nil to constants, as it makes very little sense to me.

Hm, naming for magic literal, like magic number? 🤔 While I also don't have the use case for assigning nil to a constant, So I agree that it's worth considering.

@@ -14,6 +14,12 @@
RUBY
end

it 'does not register an offense and corrects when `&.` is used for snake case const receiver' do
expect_no_offenses(<<~RUBY)
FOO&.do_something
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, I guess there's a small change that a class name will be one word, all caps - e.g. URL, URI, etc. I guess we can leave it as is, but are test cases for CONST_NAME and Const_name, so it's clearer what we are testing for exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I've updated the test cases.

…ation`

Fixes rubocop#12271.

This PR fixes a false positive for `Lint/RedundantSafeNavigation`
when using snake case constant receiver.

Camel case is used for naming classes and modules, while snake case is used for all other constants.
This naming conforms with `Naming/ConstantName` cop.

Since constants that turn into classes or modules are normally not `nil`, they will continue to be detected.
However, this PR will update to allow safe navigation for constants in snake case.

This change resolves both issue rubocop/rubocop-rails#1104 and rubocop#12271.
@koic koic force-pushed the fix_a_false_positive_for_lint_redundant_safe_navigation branch from a974a5a to 2c1ee21 Compare October 13, 2023 06:20
@bbatsov bbatsov merged commit 12016d8 into rubocop:master Oct 13, 2023
28 checks passed
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 13, 2023

Thanks!

@koic koic deleted the fix_a_false_positive_for_lint_redundant_safe_navigation branch October 13, 2023 07:45
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.

RedundantSafeNavigation does not consider constants may be nil.
2 participants