bpo-42392: Update code after merge review from 1st1 #23499
Conversation
|
@1st1 Can you please review this PR and add |
b7587f5 to
65e5422
Compare
Remove unnecessary _ger_running_loop patching
|
@aeros Can you please review this PR? |
There was a problem hiding this comment.
Mostly LGTM, the approach looks solid.
My only suggested improvement would be for _verify_parameter_is_marker, since from the current name its purpose in the __init__ of the primitive classes is not clear at a glance. Here's a couple of possible solutions:
A) Renaming to _verify_no_loop and changing it to:
def _verify_no_loop(obj, loop):
if loop is not _marker:
raise TypeError(
f'As of 3.10, the *loop* parameter was removed from '
f'{type(obj).__name__}() since it is no longer necessary'
)B) Add a exc_msg parameter, so that it's clear what it's being used for. This would look something like:
def _verify_parameter_is_marker(parameter, exc_msg):
if parameter is not _marker:
raise TypeError(exc_msg)and then used like:
def __init__(self, ...):
exc_msg = 'As of 3.10, the *loop* parameter was removed from '
f'{type(self).__name__}() since it is no longer necessary'
_verify_parameter_is_marker(loop, exc_msg)Note that this also removes the necessity of the obj parameter since it's only used for getting the function name.
I'm more in favor of (A) since it seems unlikely to do a similar mass removal of a parameter in the foreseeable future for asyncio and is the most simple, but would be fine w/ (B) since it would still be similarly readable.
|
@aeros I am more in favor of (A) but (B) is also great because it is a more explicit approach. Can you please review PR again? |
|
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 |
…ythonGH-23499) * Update code after merge review from 1st1 * Use a sentinel approach for loop parameter Remove unnecessary _get_running_loop patching * Use more clear function name (_verify_parameter_is_marker -> _verify_no_loop) * Add init method to _LoopBoundMixin to check that loop param wasn't used
Update code after merge review from @1st1.
Base PR is #23420
https://bugs.python.org/issue42392