-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ruff] Handle extra arguments to deque (RUF037)
#18614
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
Conversation
|
| 65 65 | def f(): | ||
| 66 66 | deque([], *[10]) # RUF037 but no fix | ||
| 67 |- deque([], **{"maxlen": 10}) # RUF037 | ||
| 67 |+ deque(**{"maxlen": 10}) # RUF037 |
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.
Very nit picky, but I think this is unsafe if the dict contains an iterable keyword. The same if there are any other keywords around:
>>> from collections import deque
>>> a = deque(iterable=[1, 2])
>>> list(a)
[1, 2]
>>> list(a)
KeyboardInterrupt
>>> a = deque([1], iterable=[1, 2])
Traceback (most recent call last):
File "<python-input-6>", line 1, in <module>
a = deque([1], iterable=[1, 2])
TypeError: argument for deque() given by name ('iterable') and position (1)
We should mark the fix as unsafe if there are any other arguments.
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.
Wow nice catch! I added this case:
def f():
deque([], iterable=[])and realized that I was mishandling the range passed to remove_argument because this was returning an Err! I was only passing the ArgOrKeyword::value not the whole ArgOrKeyword.
I fixed that, and then also marked the fix unsafe if we find an unrecognized number of arguments.
Summary
Fixes #18612 by:
*args, which I don't think we can fix reliablyEdit::deletionfromremove_argumentinstead of anEdit::range_replacementin the presence of unrecognized keyword argumentsI thought we could always switch to the
Edit::deletionapproach initially, but it caused problems whenmaxlenwas passed positionally, which we didn't have any existing tests for.The replacement fix can easily delete comments, so I also marked the fix unsafe in these cases and updated the docs accordingly.
Test Plan
New test cases derived from the issue.
Stabilization
These are pretty significant changes, much like those to PYI059 in #18611 (and based a bit on the implementation there!), so I think it probably makes sense to un-stabilize this for the 0.12 release, but I'm open to other thoughts there.