-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
gh-94912: Added marker for non-standard coroutine function detection #99247
Merged
Merged
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
4cf8e98
Added initial test and docs drafts.
carltongibson 6b8fa87
Make first case pass setting code flags.
carltongibson fa22a16
Adjust iscoroutinefunction() to look for async __call__.
carltongibson 513d358
📜🤖 Added by blurb_it.
blurb-it[bot] 397a975
Moved to decorator; extra tests.
carltongibson d156eab
Added tests for lambda, @classmethod, and @staticmethod cases.
carltongibson 34859de
Adjusted docs signature for review comment.
carltongibson 47a9fea
Improve grammar in NEWS file
gvanrossum cd6c491
Adjusted docs.
carltongibson 629dd81
Updated for review comments.
carltongibson a518c0f
Merge branch 'main' into fix-issue-XXXXX
gvanrossum c6d2b88
Use marker for implementation.
carltongibson b7249a8
Added "What's New..." entry.
carltongibson 3bc72e2
Adjusted implementation.
carltongibson c073cf7
Renamed to _is_coroutine_marker.
carltongibson 5ffba32
Adjusted implementation and docs.
carltongibson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Library/2022-11-17-10-02-18.gh-issue-94912.G2aa-E.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Add :func:`inspect.markcoroutinefunction` decorator which manually marks | ||
a function as a coroutine for the benefit of :func:`iscoroutinefunction`. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For the check, we need to check plain functions and the
__func__
cases, and the__call__
implementation for class instances. (Tests cover that: given that functions have a__call__
we need to be careful not to just check the one case.)I looked at various ways of combining this into a single pass, but none that I found were particularly clear. Happy to take a suggestion, but I don't see this as being performance sensitive.
The implementation here is more sophisticated that the older
asyncio
one…https://github.com/python/cpython/blob/3.11/Lib/asyncio/coroutines.py#L17-L24
… but (as per the added test cases) various extra (seemingly legitimate) cases are now covered. (So 👍 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.
I'm thinking about this, but being distracted by other stuff. I'll try to get to it later this week.
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.
Hey @gvanrossum — thanks.
I thought one maybe neater re-write looks like this:
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.
So perhaps a confusing aspect is that everything that's callable has a
__call__
method. Also the presence of__func__
indicates it's a class or instance method.At least, those things seem to be obfuscating what's going on a bit.
Also, looking at the code for
_has_code_flag()
, I wonder if we don't need the same logic for checking the_is_coroutine
flag.What would happen if we wrote a helper function like this?
(FWIW I wonder if the variable
_is_coroutine
shouldn't be renamed_is_coroutine_marker
.)Now, I'm not entirely sure why you're testing for
__call__
, when none of the otheris<something>function()
predicates test for it. Maybe things would become simpler if we removed that feature?We could then define our function like this:
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.
OK, so the issue is we have classes like this:
... where we need instances to be identified as async, and
iscoroutinefunction
doesn't (currently) pick it up.Note that this is an actual, already-out-there, need. e.g. the Starlette framework has this exact check to work around this limitation.
asgiref
works around this limitation in its usage by marking the instance itself with the_is_coroutine
marker.Either way, I think it is desirable to return
True
for class instance of classes such asCl
with anasync def __call__()
So that's the reason for adding the additional
... _has_code_flag(obj.__call__, ...)
check.In this PR, given that we were adding
@staticmethod
and@classmethod
and so on, it seemed that we also should cover this kind of case:Which leads to needing similar in the
_is_coroutine_marker
block.The latter case is somewhat hypothetical... — it comes from trying to cover all the cases here, rather than from real-use™. To that extent I'd be happy to drop it, but I guess if we do there's an issue in X months time, saying "I need to do this". 🤔
I don't know the correct thing to say. (?)
Yep OK. +1
Let me have a read.
Thanks so much for your thought and input on this.
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.
Hum, I still feel that you're sneaking in a new feature under the radar. :-) What this PR is supposed to fix is just to add the ability to mark a sync function as "morally async". IOW the behavior of
should be the same as
and since the latter prints
False
, I don't think this PR has any business changing the output toTrue
.If you think the functionality used by Starlette deserves to be included in inspect, you should probably open a new issue for that first so it can be discussed properly. But I personally think this is overreaching (however, I am repeating myself).
Have you considered adding
def _has_coroutine_mark(f):
as I suggested?(Sorry to be such a pain. But once we let this in there's no way back, so I prefer to have the minimal necessary functionality only. Other core devs may differ though.)