-
Notifications
You must be signed in to change notification settings - Fork 578
Fix build for Python 3.14 #638
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
In extreme cases, isinstance(..., ()) may still return False instead of raising a TypeError
This uses private API now. Proper fix pending discussion in python/cpython#131148
* Add deprecation in docstring * Stop using private API, refs python/cpython#131148 * Use AbstractEventLoop instead, after @Vizonex's suggestion
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.
@graingert @fantix
I like the fact that my approach from winloop is being utilized here but would it be smart if the install()
function threw an error if 3.16+ is used?
I'm not sure, but I feel like it's fine that |
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.
Looks good. I'll have to mirror some of this myself even though I don't agree with all of it but it works nevertheless. You'll have to let 1st1 review it since I'm not the deciding factor but wanted to provide input and feedback to the project since I run the windows version of it.
cdef aio_set_running_loop = getattr(asyncio, '_set_running_loop', None) | ||
cdef aio_debug_wrapper = getattr(asyncio.coroutines, 'debug_wrapper', None) | ||
cdef aio_AbstractChildWatcher = asyncio.AbstractChildWatcher | ||
cdef aio_AbstractChildWatcher = getattr(asyncio, "AbstractChildWatcher", ()) |
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.
Why ()
and not just None
?
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.
()
is a type, but None
is not. We have usages like isinstance(..., aio_AbstractChildWatcher)
and ()
returns False instead of TypeError
.
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.
()
is shorthand for tuple()
Much like {}
is shorthand for dict()
from ._version import __version__ # NOQA | ||
|
||
|
||
__all__ = ('new_event_loop', 'install', 'EventLoopPolicy') |
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.
is _typing.Tuple[str, ...]
necessary here?
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.
Yeah the later __all__ += (...)
would break typing
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.
Good stuff, i've only two nit comments
Changes ======= * Fixes for Python 3.14 (#638) (by @graingert @hroncok @paulocheque @fantix in 46456b6 for #637) Fixes ===== * Use Cython `enum` for `__PREALLOCED_BUFS` (#634) (by @jakirkham in 7bb12a1 for #634) * test: fix getaddrinfo test (#663) (by @fantix in 5680792 for #663) * test: fix task name for Python 3.13.3/3.14 (#662) (by @cjwatson in 96b7ed3 for #662)
Fixes #637