-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Modernize and fix CI #121
Modernize and fix CI #121
Conversation
Test on 3.10, 3.11, 3.12-dev, and recent pypy. Port from MultiError to ExceptionGroup, thus supporting (and requiring) Trio v0.22.0+. Drop support for 3.6 (required by ExceptionGroup support).
857e23e
to
80d6ff1
Compare
Trio-Asyncio requires at least Python 3.6. It is tested on recent versions of | ||
3.6, 3.7, 3.8, and nightly. | ||
Trio-Asyncio requires at least Python 3.7. It is tested on recent versions of | ||
3.7 through 3.11, plus 3.12-dev. |
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.
3.12 isn't tested on CI though? Maybe that should be added to the config?
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.
It's tested on Ubuntu. The test matrix is basically copied from current Trio; I don't know how to do prereleases on other platforms on GHA, and I don't think they'd add much additional coverage. We can add them once 3.12 final is out.
ci.sh
Outdated
@@ -72,14 +38,6 @@ python -m pip --version | |||
python setup.py sdist --formats=zip |
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.
Maybe use a PEP 517 build process (python -m build -s
), as invoking python setup.py
is deprecated?
Alternatively, pip install .
would probably be simpler while still being modern.
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.
Changed to pip install .
bump! Is there anything preventing this being merged? |
Just the fact that no one has approved it. |
Since @agronholm has reviewed I'll let him approve if he's not too busy... |
I may be a little out of my depth here but I'll see if I can give this a proper review over the weekend. |
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 didn't find anything wrong; just a note about Python version classifiers.
@@ -91,7 +87,6 @@ | |||
"Programming Language :: Python :: Implementation :: CPython", | |||
"Programming Language :: Python :: Implementation :: PyPy", | |||
"Programming Language :: Python :: 3 :: Only", | |||
"Programming Language :: Python :: 3.6", | |||
"Programming Language :: Python :: 3.7", |
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.
Maybe add more Python version classifiers?
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.
This looks good, but I would feel more comfortable if someone more familiar with the internals of asyncio had a look at this as well.
Code changes:
open_loop()
when using an async loop, instead of just claiming to. Raising them out ofdefault_exception_handler
doesn't work because asyncio catches it. Instead we now create a new task in the loop nursery to raise the exception.add_reader
oradd_writer
on a socket object (rather than fd) could produce an uncaught exception if the reader/writer were unregistered and the socket closed inside the same reader/writer callback.asyncio.current_task
on 3.12+ so that it looks at the current_loop contextvar instead of just the C-level running loop cache. This is now necessary becausecurrent_task
is now implemented in C and thus bypasses our monkeypatches by default.Test changes:
Closes #119, #120.