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

Rework Lint/RedundantSafeNavigation to be more safe #8884

Merged
merged 1 commit into from Oct 12, 2020

Conversation

fatkodima
Copy link
Contributor

Closes #8867
Closes #8868

I reworked this to make an offense only when used inside conditions and only for whitelisted methods.

cc @bbatsov @marcandre @natematykiewicz

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Good work.

I would remove nil? from the allowlist, since foo&.nil? is trivially falsey but foo.nil? isn't.

Unlikely, but nil.methods - Object.new.methods #=> [:to_h, :&, :to_c, :to_f, :to_i, :to_a, :to_r, :rationalize, :|, :^], so foo&.respond_to?(any_of_these) shouldn't be flagged. Because of this, probably best to mark the autocorrect as unsafe (foo&.respond_to?(variable) can't be autocorrected safely).

@fatkodima fatkodima force-pushed the rework-redundant_safe_navigation branch from b017f09 to 4fc4956 Compare October 10, 2020 17:19
@fatkodima
Copy link
Contributor Author

Good points 👍 Removed nil? and marked as unsafe.

@marcandre
Copy link
Contributor

Great. Would it be difficult to exclude foo&.respond_to?(:to_a) and others, when the argument is a literal symbol corresponding to one of the few methods that NilClass defines that may not be defined on other objects?

@fatkodima fatkodima force-pushed the rework-redundant_safe_navigation branch from 4fc4956 to d03a9c2 Compare October 10, 2020 17:48
@fatkodima
Copy link
Contributor Author

Updated with suggestion.
This surely was easy, I just was lazy 😄

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Almost ready

spec/rubocop/cop/lint/redundant_safe_navigation_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/lint/redundant_safe_navigation_spec.rb Outdated Show resolved Hide resolved
config/default.yml Show resolved Hide resolved
docs/modules/ROOT/pages/cops_lint.adoc Show resolved Hide resolved
@fatkodima fatkodima force-pushed the rework-redundant_safe_navigation branch from d03a9c2 to 8889b09 Compare October 10, 2020 20:18
@fatkodima
Copy link
Contributor Author

@marcandre Thank you for all reviews 💪
Updated with your suggestions.

@fatkodima fatkodima force-pushed the rework-redundant_safe_navigation branch from 8889b09 to 6717823 Compare October 11, 2020 10:18
@fatkodima
Copy link
Contributor Author

Updated with suggestions. Let me know if there is anything else I can improve.

@bbatsov bbatsov merged commit c40307a into rubocop:master Oct 12, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 12, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants