bpo-42392: Remove loop parameter form asyncio locks and Queue #23420
Conversation
eea500d to
152ac8a
Compare
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
|
@asvetlov: Please replace |
| self._waiters = collections.deque() | ||
| self._value = False | ||
| if loop is None: |
There was a problem hiding this comment.
I'd actually keep the loop=None parameter and add something like this:
if loop is not None:
raise TypeError(
f'passing explicit "loop" argument to {type(self).__name__}() '
f'is no longer necessary')There was a problem hiding this comment.
I thought that we should fully remove loop parameter because it deprecated.
Maybe it's better to fully remove loop as for me it a lit bit confusing to have a parameter that raises an exception when it set, it looks the same as we won't have loop parameter when it set we will receive an error that init method doesn't accept loop but it has a drawback - exception message won't be such explicit.
What is your opinion?
There was a problem hiding this comment.
We could do marker=object() and then use the loop=marker de
8000
fault to make it better. But I'm also not 100% about my own suggestion here :) @asvetlov thoughts?
There was a problem hiding this comment.
I'm +1 for raising an explicit error and using the sentinel approach to check if the user passed a loop arg. Especially with how widespread the loop parameter is used throughout existing asyncio code, it's going to be a much smoother transition to have an error message that tells users precisely what they did incorrect rather than the parameter just disappearing.
(Also keeping in mind the unfortunate reality that not everyone tests with deprecation warnings on, or might not have gotten around to removing loop throughout their own code since the warnings started in 3.8)
As for when the parameter should just be removed entirely without an explicit error message, I think doing that in 3.11 or 3.12 would be fine.
There was a problem hiding this comment.
Now I get it, sorry that didn't understand it the first time.
Give an explicit error will be better for debugging and will make the transition much easier.
I will update #23499 to use this approach regarding the loop parameter.
There was a problem hiding this comment.
Although I would make the message a little bit more direct and technically specific:
if loop is not None:
raise TypeError(
f'As of 3.10, the *loop* parameter was removed from {type(self).__name__}() ')
f'since it is no longer necessary')There was a problem hiding this comment.
loop=marker looks a little more protective but, I think, the whole idea is a slight overcomplication.
For me, "removal" means exactly removal but I can live with the proposed change.
|
@1st1 I will open a new PR and will try to update everything regarding your comments |
Thanks! |
…#23420) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
|
I just came across this PR and I am curious why |
|
@gvanrossum Please, take a look here - https://bugs.python.org/msg381389 |
|
Thanks -- I found it more useful to look at the message in context (https://bugs.python.org/issue42392#msg381389). A lot of the thread seems dedicated to discussing the need for a global thread-based lock. |
|
Aha, I found the key concern, in https://bugs.python.org/issue42392#msg381329:
My curiosity is now satisfied, thanks for indulging me! |
Remove loop parameter from
__init__in allasyncio.locksandasyncio.Queueclasses.@1st1 @asvetlov I didn't update because want to hear your opinion regarding the implementation.
https://bugs.python.org/issue42392