add include/exclude check to codemods that did not have it #208
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
=======================================
Coverage 96.25% 96.26%
=======================================
Files 89 89
Lines 4089 4097 +8
=======================================
+ Hits 3936 3944 +8
Misses 153 153
|
| def update_attribute(self, true_name, original_node, updated_node, new_args): | ||
| del true_name, original_node | ||
| del true_name | ||
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node | ||
|
|
There was a problem hiding this comment.
The filtering call here is not needed as it is covered by the leave_Call in the parent class.
| def update_simple_name(self, true_name, original_node, updated_node, new_args): | ||
| del true_name | ||
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node | ||
|
|
src/core_codemods/use_walrus_if.py
Outdated
| def leave_Assign(self, original_node: cst.Assign, updated_node: cst.Assign): | ||
| del updated_node | ||
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node | ||
|
|
There was a problem hiding this comment.
As we discussed before, we only filter the nodes that "anchor" the changes. In this case the If node is the one being used for this purpose. Note that the change is reported with the If node position. Thus you don't need this check.
src/core_codemods/use_walrus_if.py
Outdated
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node | ||
|
|
There was a problem hiding this comment.
Same as the other comment.
clavedeluna
force-pushed
the
incl-exl
branch
3 times, most recently
from
876836a to
c618a62
Compare
| self.node_position(original_node) | ||
| ): | ||
| return original_node | ||
|
|
There was a problem hiding this comment.
in attempting to add a unit test here I discovered that this include/exclude check will not work because a function def node has a different start/end numbers. Here's an example:
For code
from django.dispatch import receiver
@csrf_exempt
@receiver(request_finished)
def foo():
pass
If a user wanted to exclude a specific line here, say they pass in to exclude ln5 which is line def foo(): (not even the line for the decorator), when we hit leave_FunctionDef, we need to match pos.start.line == line and pos.end.line == line. However, the position for this leave_FunctionDef node is CodeRange(start=CodePosition(line=5, column=0), end=CodePosition(line=6, column=8)) (ends at ln6)
This is evidence that we need much more robust include/exclude behavior for these types of cases that are more subtle. I'm not quite prepared to implement this yet. @drdavella thoughts? Should I remove this include/exclude check for this (and any other case I find has this issue) for now?
There was a problem hiding this comment.
@clavedeluna yes you can remove those checks for now but please make sure to add TODOs. None of our tooling is currently taking advantage of line-based filtering as far as I'm aware so the impact should be minimal for now.
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node |
There was a problem hiding this comment.
Sorry for not realizing this before, but we should return updated_node here just in case some node within the original_node subtree was changed. Otherwise we may revert those previous changes.
I don't think it will affect the current codemods that contains this snippet, but this will probably be copy/pasted a lot, so it should include the "safe" version.
There was a problem hiding this comment.
I noticed we don't always return updated_node like the docs indicate we should. I kinda wanted this to read "if we are excluding this node bc of line, don't change". so returning original_node reads "no change happened" but it's true that's not always the case.
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(_original_node) | ||
| ): | ||
| return _original_node |
src/core_codemods/use_generator.py
Outdated
| if not self.filter_by_path_includes_or_excludes( | ||
| self.node_position(original_node) | ||
| ): | ||
| return original_node |
|
Overview
Description
leave_Nodemethodsfilter_by_path_includes_or_excludes, but this is harder than it looks. I was hoping to add it to the existing BaseTest class so any new codemods would benefit from it. I have some basic code that doesn't work very well, so maybe I can iterate on it later on.