chore: move CancelledError and InvalidStateError to tasks #55
Conversation
asyncio/task.py
Outdated
| class CancelledError(BaseException): | ||
| """Injected into a task when calling `Task.cancel()`""" | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| class InvalidStateError(Exception): | ||
| """Can be raised in situations like setting a result value for a task object that already has a result value set.""" | ||
|
|
||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
You could define these conditionally by testing whether they are in _asyncio, using hasattr(). I don't know whether you would then want to define them in task.py or core.py.
There was a problem hiding this comment.
I can do that, sure.
To clarify, I was kind of doing this already by the from _asyncio import CancelledError, InvalidStateError. In the case of the _asyncio existing but missing the exceptions (because of a mismatched version of CircuitPython?) an AttributeError is raised, leading to pulling them from task instead.
Even with that in mind, these should use hasattr()?
I don't know whether you would then want to define them in task.py or core.py.
Where they live is somewhat inconsequential, as long as they're loaded when they don't exist in CircuitPython proper. An alternative was that they'd live under an exceptions.py.
There was a problem hiding this comment.
I was thinking these exceptions are defined unconditionally in task.py right now, and that they could be conditionalized. But now I'm realizing that task.py is not used at all if _asyncio exists. Is that right? If so, then I'm off the mark.
There was a problem hiding this comment.
If _asyncio exists, that's right.
With this new change, if _asyncio exists but doesn't have the two new errors we'll reach into task.py and grab these two errors. That does mean that we'll interpret the rest of task.py -- which I'm realizing now might be undesirable to save on memory usage.
If you think that's a concern I can move them to their own conditionally imported file
There was a problem hiding this comment.
which I'm realizing now might be undesirable?
Yes, it would be a waste of time and RAM space if it gets imported. So maybe just define them inside one of the arms of the import try-except, or factor them into exceptions.py as you suggested.
There was a problem hiding this comment.
Updated per request - it's in the import try-except because I didn't want to move the TimeoutError
|
@imnotjames Do you want to merge all your PR's at the same time? I approved but did not merge because I don't know how you want to stage the changes. |
|
Tests would be ideal first if possible, but not for any particular reason. If we want to close them out then this one is fine on its own. Everything else not in draft can be any order. |
|
Given there seems to be more to hash out on the testing side of things, this can probably be merged in now which unblocks changes in the C code |
Updating https://github.com/adafruit/Adafruit_CircuitPython_AD569x to 2.0.1 from 2.0.0: > update docs Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 6.1.0 from 6.0.2: > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#182 from RetiredWizard/main Updating https://github.com/adafruit/Adafruit_CircuitPython_asyncio to 1.2.1 from 1.2.0: > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#55 from imnotjames/feat/53/cancelled-and-invalid-state Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
this moves
CancelledErrorand createsInvalidStateErrorwithin tasks to support #54 & adafruit/circuitpython#8576 -- and done in a way so that if we were to include these in_asynciothat we use the C exceptions betterThis is being done in a separate PR so that the CircuitPython tests that run will get the expected
CancelledErrorand operate as expected.Open to other suggestions, though!