GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-81371: implementing checking async generator methods by wrongnull · Pull Request #102129 · python/cpython

Conversation

@wrongnull

@wrongnull wrongnull commented Feb 21, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR may fix #81371

  • Issue: asyncio.iscoroutinefunction(<async_generator>.asend) returns False #81371

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@wrongnull
implementing checking async generator methods
a8c7489
This PR may fix python#81371
@bedevere-bot bedevere-bot added the awaiting review label Feb 21, 2023
@bedevere-bot

bedevere-bot commented Feb 21, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wrongnull wrongnull changed the title implementing checking async generator methods gh-81371: implementing checking async generator methods Feb 22, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 22, 2023
asyncio.iscoroutinefunction(<async_generator>.asend) returns False #81371
Closed
blurb-it Bot and others added 2 commits February 22, 2023 02:14
@blurb-it
📜🤖 Added by blurb_it.
f309fbd
@wrongnull
handle one more corner case
d16fa1a
well, now I'm not sure if this changes matches your excpectations about the way how stdlib should look like
@wrongnull

wrongnull commented Feb 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn
sobolevn requested changes Feb 22, 2023
View reviewed changes

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

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:

  1. There are other ways to do that. Flags, coroutine marks, etc. Why this one is the best one? (I am not an asyncio expert)
  2. Is it a problem worth fixing at all?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Misc/NEWS.d/next/Library/2023-02-22-02-14-38.gh-issue-81371.tUo0u1.rst
@@ -0,0 +1 @@
This PR may fix the issue 81371

@sobolevn sobolevn Feb 22, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@wrongnull wrongnull Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/inspect.py
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

@sobolevn sobolevn Feb 22, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@wrongnull wrongnull Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 22, 2023
@carljm

carljm commented Feb 22, 2023

Copy link
Copy Markdown
Member

Is there one certain way to define if callable returns awaitable?

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm
carljm requested changes Feb 22, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@wrongnull

wrongnull commented Feb 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Is there one certain way to define if callable returns awaitable?

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.

But there is no way to add attribute to builtin function without monkey patching. Do we need new type(s) to this? Such as builtin_coroutine_function_or_method
Or at least add special flag to already exists ones

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wrongnull

wrongnull commented Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

What if PyCFunctionObject structure contains flags that match co_flags field in code object? Or will they carry the same meaning

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Feb 23, 2023

Copy link
Copy Markdown
Contributor

This isn't the way to fix this, I am closing this, please continue the discussion on the issue.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303 kumaraditya303 closed this Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm requested changes
@sobolevn sobolevn sobolevn requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

asyncio.iscoroutinefunction(<async_generator>.asend) returns False

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@wrongnull @bedevere-bot @carljm @kumaraditya303 @sobolevn
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.