Replace deprecated use of MultiError with exceptiongroup#679
Conversation
penguinolog
left a comment
penguinolog
left a comment
There was a problem hiding this comment.
Small changes needed and LGTM
|
|
||
| # TODO(Aleksei): trio.MultiError is deprecated in favor of exceptiongroup package usage and `Except *` | ||
| with trio.MultiError.catch(self._handle_main_loop_exception): | ||
| with exceptiongroup.catch({BaseException: self._handle_main_loop_exception}): |
There was a problem hiding this comment.
This change needs lower constraints for trio version
There was a problem hiding this comment.
Oops, good point.
Do you mean just adding a constraint on the dependency, or do you want it to work on old versions of trio?
I can implement a shim so that this uses exceptiongroup.catch on new versions of trio and MultiError.catch on older ones if you want. It's not super hard, but does open up the annoying question of how to test on multiple versions of trio.
There was a problem hiding this comment.
Personally I prefer constraints on requirements, because support of unlimited legacy versions is an unlimited pain.
There was a problem hiding this comment.
Yup, I think this makes more sense too. I just wasn't sure what your preferences were. I've now pushed a version with a constraint on the earliest version of trio where this should work.
| glib = ["PyGObject"] | ||
| tornado = ["tornado"] | ||
| trio = ["trio"] | ||
| trio = ["trio>=0.22.0", "exceptiongroup"] |
There was a problem hiding this comment.
Please also add exceptiongroup to the functional tests requirements. Without it tests fail
There was a problem hiding this comment.
Whoops, sorry. I'd only run the tests on Python 3.10 so hadn't noticed that they failed on later Python versions. Now pushed a fix.
Pull Request Test Coverage Report for Build 6996535478
💛 - Coveralls |
penguinolog
added
Feature
3 participants
Checklist
masterorpython-dual-supportbranchtoxsuccessfully in local environmentDescription:
This pull request fixes a deprecation warning when running with recent versions of trio (and a TODO comment from the relevant section of the code) by replacing usage of trio's
MultiError.catchwithexceptiongroup.catch. It required a little bit of fiddling to get the right semantics, but as far as I can tell this should now be identical behaviour without the deprecation warning (and it passes all the tests)This does have the possibly slightly undesirable feature of adding a dependency on exceptiongroup for using trio. I think this is mostly harmless - it's a dependency of trio anyway in Python versions prior to
except*syntax being added. If you want I can do a slightly more complicated version of this pull request that removes the dependency on Python versions recent enough to not need it, but that seemed likely to be more trouble than it was worth compared to just installing the backport unconditionally.