-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-81371: implementing checking async generator methods #102129
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
This PR may fix python#81371
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
well, now I'm not sure if this changes matches your excpectations about the way how stdlib should look like
Is there one certain way to define if callable returns awaitable? I mean it is possible to reimplement some logic in C. For instance callable can has special flag to this purpose. It's because I'm not sure if my patch handles all possible corner cases so maybe there are more subtle ones. So if the checked object was not declared with async def the only way to check this condition is brute force it manually, I guess. |
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.
Couple of thoughts:
- There are other ways to do that. Flags, coroutine marks, etc. Why this one is the best one? (I am not an asyncio expert)
- Is it a problem worth fixing at all?
@@ -0,0 +1 @@ | |||
This PR may fix the issue 81371 |
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.
May? Or does it? Please, also specify what the issue is. These are user-facing notes. It must be clear without a github link.
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.
May? Or does it? Please, also specify what the issue is. These are user-facing notes. It must be clear without a github link.
I'm not 100% sure if it does. In the original issue those methods just returns some awaitable objects that doesn't pass the check. And I fixed it, but only for this certain example (I will add the tests soon). The obstacle is that I don't know if there are similar functions somewhere in the inerpreter. I'm not an internals expert. It would be great if you or someone else could suggest me such places.
def iscoroutinefunction(obj): | ||
"""Return true if the object is a coroutine function. | ||
|
||
Coroutine functions are normally defined with "async def" syntax, but may | ||
be marked via markcoroutinefunction. | ||
""" | ||
#check if obj is async gen method and returns awaitable |
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.
Please, at least add tests for this feature.
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.
as soon as I fully understand the original question
See #81371 (comment) I don't think the current approach in this PR is the right approach; we shouldn't be checking against an approved list of method names. |
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.
See above comment.
But there is no way to add attribute to builtin function without monkey patching. Do we need new type(s) to this? Such as |
What if |
This isn't the way to fix this, I am closing this, please continue the discussion on the issue. |
This PR may fix #81371