-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added literal-or-new-object-identity codemod #186
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 96.25% 96.28% +0.03%
==========================================
Files 81 82 +1
Lines 3795 3826 +31
==========================================
+ Hits 3653 3684 +31
Misses 142 142
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with comments. My main substantive comment/question is about expressions with literals on the LHS.
src/core_codemods/docs/pixee_python_literal-or-new-object-identity.md
Outdated
Show resolved
Hide resolved
def leave_Comparison( | ||
self, original_node: cst.Comparison, updated_node: cst.Comparison | ||
) -> cst.BaseExpression: | ||
if self.filter_by_path_includes_or_excludes(self.node_position(original_node)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized I've forgotten this filter in many of my own recent codemods so I'll need to go back and make updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason to have another look at the API. This should be automated in these simple cases.
case cst.Comparison( | ||
left=left, comparisons=[cst.ComparisonTarget() as target] | ||
): | ||
if isinstance(target.operator, cst.Is | cst.IsNot): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be ever so slightly cleaner if you match here? But possibly I'm just obsessed with match right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I'm not using the operator in any place. Using match would add an extra indentation which is why I avoid in those simple cases.
One additional comment but Sonar calls this out as a non-compliant example as well: mylist = [] # mylist is assigned a new object
param is mylist # Noncompliant: always False I know it's harder for us to do constant propagation without semgrep but we might want to add a ticket for this. |
Quality Gate failedFailed conditions C Reliability Rating on New Code (required ≥ A) See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
I'll do this in a future PR. We kinda have the solution to that already but I need to do some testing here. |
Overview
Added a new codemod that replaces
is
andis not
comparisons with literals and new objects with==