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

Enhance Lint/RedundantSafeNavigation to handle conversion methods with defaults #12418

Conversation

fatkodima
Copy link
Contributor

Fixes #12412.

With these changes I found a few offenses in the gitlab's codebase.

# foo&.to_h || {}
#
# # good
# foo.to_h
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to document other literals as well.

range = range_between(node.lhs.loc.dot.begin_pos, node.source_range.end_pos)

add_offense(range) do |corrector|
corrector.replace(node, "#{receiver.source}.#{method_name}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
corrector.replace(node, "#{receiver.source}.#{method_name}")
corrector.replace(range, ".#{method_name}")

it 'registers an offense and corrects when `.&` is used in `.to_h` coversion with default' do
expect_offense(<<~RUBY)
foo&.to_h || {}
^^^^^^^^^^^^ Redundant safe navigation detected.
Copy link
Member

@koic koic Nov 26, 2023

Choose a reason for hiding this comment

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

In such cases, adjusting the message a bit would likely prevent user confusion. e.g.:

Suggested change
^^^^^^^^^^^^ Redundant safe navigation detected.
^^^^^^^^^^^^ Redundant safe navigation with default literal value detected.

@fatkodima fatkodima force-pushed the redundant_safe_navigation-conversions-with-defaults branch from 397ce9b to 2429636 Compare November 26, 2023 10:38
@fatkodima
Copy link
Contributor Author

Updated.

@ydakuka
Copy link

ydakuka commented Nov 26, 2023

The example is missing:

# bad
hsh&.to_h { |k, v| [k, v] } || {}
# good
hsh.to_h { |k, v| [k, v] }

With these changes I found a few offenses in the gitlab's codebase.

This should fix the following offenses: https://github.com/gitlabhq/gitlabhq/blob/master/lib/security/ci_configuration/sast_build_action.rb#L24

@fatkodima fatkodima force-pushed the redundant_safe_navigation-conversions-with-defaults branch from 2429636 to fe5ed60 Compare November 26, 2023 15:32
@koic koic merged commit cb43b34 into rubocop:master Nov 27, 2023
28 checks passed
@koic
Copy link
Member

koic commented Nov 27, 2023

Thanks. Let's proceed with this first.

@fatkodima fatkodima deleted the redundant_safe_navigation-conversions-with-defaults branch November 27, 2023 09:27
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: Redundant safe navigation for basic types
3 participants