Closed jdb78 closed 3 years ago
Thanks for the PR!
However, I'm not sure I follow the logic. You've visited this much more recently than me, so some questions:
node.value
is the object part of the Attribute
AST node, taken from the AST as-is, without any analysis.
get_attribute
, resolve_attribute
.Attribute
node somehow? Or does this happen automatically for some reason I'm missing here?Thanks for the PR!
Thank you for the review!!
However, I'm not sure I follow the logic. You've visited this much more recently than me, so some questions:
Here
node.value
is the object part of theAttribute
AST node, taken from the AST as-is, without any analysis.
- Is this what we want? Pyan normally does some analysis - see
get_attribute
,resolve_attribute
.
Yes, in this case, this is what we want - we already know that this is an attribute.
- Should we handle also the attribute part of the
Attribute
node somehow? Or does this happen automatically for some reason I'm missing here?
We are handling it if it if we can resolve the object belonging to the attribute
- Are there cases where the suggested strategy might fail, and how?
I tested it on fairly large codebases and it seemed to work well - however, I believe adding tests to the codebase would be a major improvement in the future.
- Should we add a wildcard node upon failure, like the old code did?
I think for attributes this will be very verbose but happy to do that
Thanks for taking the time to consider the points in my review. Merged!
Yes, automated tests would be a major improvement. Also a major undertaking. Contributions welcome. ;)
Chained function calls are not recorded currently, e.g. for
create_class_function().method1()
create_class_function()
is not recorded. Instead of assigning a wildcard to a node in an unknown namespace, we can explore if going down that route leads back into the covered namespace.