Adds support for attr_* method references #3706
Conversation
tests. Prob can use some cleanup
…r-method-references
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
tommycrumrine
changed the title
| if @target.is_a?(MethodTarget) && (name = node.name.to_s) == @target.method_name | ||
| @references << Reference.new( | ||
| name, | ||
| node.message_loc, #: as !nil |
There was a problem hiding this comment.
Does type checking pass without this #: as !nil?
There was a problem hiding this comment.
Opps, yep something is definitely off here, will look into it. I'm a little new to the type checking 😅
There was a problem hiding this comment.
All good, let me know if you need any support on it.
There was a problem hiding this comment.
I think we do need to cast it because message_loc can be nil. I checked out the branch and type checking is failing without the narrows.
| def unescaped_argument_names(node) | ||
| return [] if node.arguments.nil? | ||
|
|
||
| node.arguments.arguments.select { |arg| arg.respond_to?(:unescaped) }.map(&:unescaped) |
There was a problem hiding this comment.
Can node.arguments.arguments be nil?
egiurleo
added
enhancement
There was a problem hiding this comment.
This is working well for when we ask for references on a call to an attribute, but when trying to find references on the attribute definition itself, we are accidentally searching for the attr_reader call instead of the symbol/string names.
class Foo
attr_reader :bar # here we search for `attr_reader` instead of bar
def something
bar # here it works well
end
endWe need to adjust the target in references.
Basically, we need to:
- Start accepting
SymbolNodeas a possible target - Check if the
node_context.call_nodeis an attribute related call when we get aSymbolNodeback - Construct the target object using the symbol value instead of the call node's name
| def unescaped_argument_names(node) | ||
| return [] if node.arguments&.arguments.nil? | ||
|
|
||
| node.arguments.arguments.select { |arg| arg.respond_to?(:unescaped) }.map(&:unescaped) |
There was a problem hiding this comment.
| node.arguments.arguments.select { |arg| arg.respond_to?(:unescaped) }.map(&:unescaped) | |
| node.arguments.arguments.filter_map do |arg| | |
| case arg | |
| when Prism::StringNode | |
| arg.unescaped | |
| when Prism::SymbolNode | |
| arg.value | |
| end | |
| end |
| if @target.is_a?(MethodTarget) && (name = node.name.to_s) == @target.method_name | ||
| @references << Reference.new( | ||
| name, | ||
| node.message_loc, #: as !nil |
There was a problem hiding this comment.
I think we do need to cast it because message_loc can be nil. I checked out the branch and type checking is failing without the narrows.
3 participants
Motivation
Shows references for methods defined with
attr_reader,attr_writer,attr_accessorRe-opening stale PR #2848. Sorry this fell off my radar!
Closes #2668
Implementation
attr_reader,attr_writer,attr_accessorare just methods, so on call node if we detect one of these methods, and one of the arguments match the target method name, add the reference.Additionally, I have ensured it works with empty and string arguments as well as added tests for these cases.
Automated Tests
Yes!
Manual Tests