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

Support Python 3.8+ in backported asyncio.staggered #96

Closed
wants to merge 3 commits into from

Conversation

vemel
Copy link

@vemel vemel commented Sep 27, 2024

What do these changes do?

Even though backported staggered.py for Python 3.12+ is used only in Python3.12, this module cannot be imported in Python 3.8. I backported an older version of staggered.py that is compatible with Python 3.8, but does not support eager task factories. I preserved type annotations and added missing ones.

I think, since aiohappyeyeballs is a py38+ project, all modules should be at least importable in py38+ for linting and type checking. Also, staggered_race function signature should now be the same for py312 and for lower python versions.

Backported from https://github.com/python/cpython/blob/16b46ebd2b0025aa461fdfc95fbf98a4f04b49e6/Lib/asyncio/staggered.py, added missing type annotations.

Are there changes in behavior for the user?

No

Related issue number

No related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Dreamsorcerer
Copy link
Member

all modules should be at least importable in py38+

I'm not sure how you reached this conclusion. It's a private module that is only imported on newer versions of Python. Why would you be trying to import the private module directly?

but does not support eager task factories.

So, it breaks the main thing that we want to fix when this module is actually used...?

if __debug__:
for d in done:
if d.done() and not d.cancelled() and d.exception():
raise d.exception() # type: ignore
Copy link
Author

Choose a reason for hiding this comment

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

Alternatively, we can do

exc = d.exception()
assert exc is not None  # noqa: S101
raise exc

@Dreamsorcerer
Copy link
Member

for linting and type checking

I don't think the internals of the library should affect your linting or type checking. If they do (and we'd see this in aiohttp anyway), then probably adding a typing stub or something is a more sensible fix.

@vemel
Copy link
Author

vemel commented Sep 27, 2024

Actually, the internals affect type checking:

mypy: /opt/hostedtoolcache/Python/3.10.15/x64/lib/python3.10/site-packages/aiohappyeyeballs/_staggered.py:98: error: invalid syntax; you likely need to run mypy using Python 3.12 or newer  [syntax]

This happens because in type checking mode if sys.version ... conditions are ignored, type checker uses the first import it finds.

@bdraco
Copy link
Member

bdraco commented Sep 27, 2024

The simplest fix to make mypy happy would be an if TYPE_CHECKING block

@vemel
Copy link
Author

vemel commented Sep 27, 2024

I am closing this PR. Fixed on my side.

@Dreamsorcerer
Copy link
Member

Yeah, seems to have passed fine on aiohttp. The only way you should see an issue like that is if you're actually type checking the library itself (which can happen when using MYPYPATH).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants