-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
Issue 1011: Non strict operation with slice #3373
base: master
Are you sure you want to change the base?
Issue 1011: Non strict operation with slice #3373
Conversation
'expression', | ||
[ | ||
# reverse | ||
'items = items[::-1]', |
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.
items =
part is not needed
class StricterSliceOperations(base.BaseNodeVisitor): | ||
"""Check for stricter operation with slices.""" | ||
|
||
def visit_Slice(self, node: ast.Slice): |
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.
def visit_Slice(self, node: ast.Slice): | |
def visit_Slice(self, node: ast.Slice) -> None: |
self._check_pop_through_slice(node) | ||
self.generic_visit(node) | ||
|
||
def _check_reverse_through_slice(self, node: ast.Slice): |
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.
def _check_reverse_through_slice(self, node: ast.Slice): | |
def _check_reverse_through_slice(self, node: ast.Slice) -> None: |
(and all other places)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3373 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 361 362 +1
Lines 11830 11881 +51
Branches 808 815 +7
=========================================
+ Hits 11830 11881 +51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
'other = items[:]', | ||
'other = items[::]', | ||
# pop | ||
'items = items[:-1]', | ||
'some, items = "some", items[:-1]', |
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.
'other = items[:]', | |
'other = items[::]', | |
# pop | |
'items = items[:-1]', | |
'some, items = "some", items[:-1]', | |
'items[:]', | |
'items[::]', | |
# pop | |
'items[:-1]', | |
'items[None:-1]', | |
'items[None:-1:1]', |
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.
Don't forget to test corner cases with None
and explicit steps.
[ | ||
# copy | ||
'items[::]', | ||
'items[:]', |
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.
Sorry, I don't udnerstand why this works. It should always raise an error. [:]
slice must be forbidden by this rule.
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.
This works because I check assign node for closest parent before add violation. Will fix it 👍
'items[0]', | ||
'items[:2]', | ||
'a = items[1::-1]', | ||
'a = items[0]', |
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.
'a = items[0]', | |
'items[0:]', |
# other | ||
'items[0]', | ||
'items[:2]', | ||
'a = items[1::-1]', |
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.
'a = items[1::-1]', | |
'items[1::-1]', | |
'items[1:4:-1]', | |
'items[:x]', | |
'items[:-x]', | |
'items[::-x]', | |
'items[::x]', |
# bad | ||
items = items[::-1] | ||
# good | ||
items.reverse() | ||
|
||
# bad | ||
other = items[:] | ||
# good | ||
other = items.copy() | ||
|
||
# bad | ||
items = items[:-1] | ||
# good | ||
items.pop() |
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.
# bad | |
items = items[::-1] | |
# good | |
items.reverse() | |
# bad | |
other = items[:] | |
# good | |
other = items.copy() | |
# bad | |
items = items[:-1] | |
# good | |
items.pop() | |
# Correct: | |
items.reverse() | |
items.copy() | |
items.pop() | |
# Wrong: | |
items[::-1] # .reverse() | |
items[:] # .copy() | |
items[:-1] # .pop() |
I have made things!
Checklist
CHANGELOG.md
Related issues
Description
Hi! I've add non strict slice operation checking. I guess there is can be some use cases that I might not have remembered. Hope we can figure this out 🤝