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

Handle implicit receivers in Style/InvertibleUnlessCondition #12711

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Feb 25, 2024

Previously, we would attempt to call node.receiver.source, but node.receiver could be nil.

An error occurred while Style/InvertibleUnlessCondition cop was inspecting /path/to/file.rb:123:4.
undefined method `source' for nil

This teaches the cop to handle implicit receivers.


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.

Previously, we would attempt to call `node.receiver.source`, but
`node.receiver` could be `nil`.

This teaches the cop to handle implicit receivers.
@sambostock sambostock force-pushed the invertible-unless-condition-with-implicit-receiver branch from 4bbd24e to 5c9d6a8 Compare February 25, 2024 18:18
@sambostock sambostock marked this pull request as ready for review February 25, 2024 18:18
@@ -99,23 +99,23 @@ def preferred_condition(node)
end
end

def preferred_send_condition(node)
receiver_source = node.receiver.source
def preferred_send_condition(node) # rubocop:disable Metrics/CyclomaticComplexity
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see a good way to avoid disabling this cop. It would require breaking apart this method, but I don't know that that would actually improve understandability.

Open to suggestions though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option might be to explicitly fallback to self as the receiver_source and then leave all the rest the same, and rely on the redundant self correction to clean it up, but I don't know if that would actually fix the complexity offense, as it still introduces the conditional, even if it's inline.

return receiver_source if node.method?(:!)

receive = receiver_source ? "#{receiver_source}." : '' # receiver may be implicit (self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to do this instead of repeating the conditional

if receiver_source
  # ...
else
  # ...
end

in three places, since it's always the same logic.

I also considered receipt as the variable name. Not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why you didn't go with the name receiver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if that would be confusing because we would then have receiver_source and receiver, but receiver would be receiver_source + '.' or nil, not the "receiver" node, as one might expect.

I suppose another option could be dotted_receiver or dotted_receiver_source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, no great options here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I did switch1 it to dotted_receiver_source though, since that's clearer than receive.

Footnotes

  1. This change was accidentally not committed, and appears in Add missing PR feedback changes from #12711 #12722 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add a case for an operator method, as the receiver would explicitly be self.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 28, 2024

Might also be good to add an example in the cop's description with an implicit receiver.

@sambostock
Copy link
Contributor Author

sambostock commented Feb 28, 2024

I've switched1 to dotted_receiver_source, and added1 an implicit receiver example to the documentation. 👍

Footnotes

  1. These changes were accidentally not committed, and appear in Add missing PR feedback changes from #12711 #12722 instead. 2

@bbatsov bbatsov merged commit 5da1905 into rubocop:master Feb 28, 2024
32 checks passed
@sambostock sambostock deleted the invertible-unless-condition-with-implicit-receiver branch February 28, 2024 15:55
sambostock added a commit to sambostock/rubocop that referenced this pull request Feb 28, 2024
sambostock added a commit to sambostock/rubocop that referenced this pull request Feb 28, 2024
@sambostock
Copy link
Contributor Author

@bbatsov Sorry, it looks like I didn't actually push my commit doing the rename and adding the examples. 🤦

I've made the changes in #12722.

bbatsov pushed a commit that referenced this pull request Feb 29, 2024
bbatsov pushed a commit that referenced this pull request Feb 29, 2024
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

2 participants