Skip to content
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

bpo-42392: Remove loop parameter form asyncio locks and Queue #23420

Merged
merged 5 commits into from Nov 24, 2020

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Nov 20, 2020

Remove loop parameter from __init__ in all asyncio.locks and asyncio.Queue classes.

@1st1 @asvetlov I didn't update because want to hear your opinion regarding the implementation.

https://bugs.python.org/issue42392

Copy link
Contributor

@asvetlov asvetlov left a comment

Looks good in general.
I have only a few minor naming concerns,

Lib/asyncio/mixins.py Outdated Show resolved Hide resolved
Lib/asyncio/mixins.py Outdated Show resolved Hide resolved
Lib/asyncio/mixins.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 20, 2020

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

uriyyo and others added 4 commits Nov 20, 2020
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
@uriyyo
Copy link
Member Author

@uriyyo uriyyo commented Nov 20, 2020

I have made the requested changes; please review again

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 20, 2020

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from asvetlov Nov 20, 2020
@asvetlov asvetlov self-assigned this Nov 23, 2020
Copy link
Contributor

@asvetlov asvetlov left a comment

Thanks!

@asvetlov asvetlov merged commit 0ec34ca into python:master Nov 24, 2020
11 checks passed
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 24, 2020

@asvetlov: Please replace # with GH- in the commit message next time. Thanks!

Lib/asyncio/locks.py Show resolved Hide resolved
self._waiters = collections.deque()
self._value = False
if loop is None:
Copy link
Member

@1st1 1st1 Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Member Author

@uriyyo uriyyo Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@1st1 1st1 Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do marker=object() and then use the loop=marker default to make it better. But I'm also not 100% about my own suggestion here :) @asvetlov thoughts?

Copy link
Member

@aeros aeros Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@uriyyo uriyyo Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@aeros aeros Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Contributor

@asvetlov asvetlov Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Lib/asyncio/mixins.py Show resolved Hide resolved
Lib/asyncio/mixins.py Show resolved Hide resolved
@1st1
Copy link
Member

@1st1 1st1 commented Nov 24, 2020

@asvetlov @uriyyo Please address my comments.

@uriyyo
Copy link
Member Author

@uriyyo uriyyo commented Nov 24, 2020

@1st1 I will open a new PR and will try to update everything regarding your comments

@uriyyo uriyyo deleted the fix-issue-42392 branch Nov 24, 2020
@1st1
Copy link
Member

@1st1 1st1 commented Nov 24, 2020

@1st1 I will open a new PR and will try to update everything regarding your comments

Thanks!

uriyyo added a commit to uriyyo/cpython that referenced this issue Nov 24, 2020
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 13, 2021
…#23420)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants