chore: use annotations from the __future__ #203
Conversation
|
Huh, but then it fails in CI. Weird. I'll move to draft and look into this more later. |
| @@ -4,7 +4,6 @@ on: | |||
| pull_request: | |||
| branches: | |||
| - main | |||
| - 7.0 | |||
There was a problem hiding this comment.
Drive-by, now that this isn't needed since the 7.0 branch has finished its job.
|
@PietroPasotti what do you think about this? I know PEP 563 is in a bit of a limbo state, but by the time that's resolved it'll be some later version of Python that supports the more minimal style of type annotations anyway, so it seems like a win to do this now? For what it's worth, I'm keen to do this in ops too. |
| ): | ||
| self._ctx = ctx | ||
| self._arg = arg | ||
| self._state_in = state_in | ||
|
|
||
| self._emitted: bool = False | ||
|
|
||
| self.ops: Optional["Ops"] = None | ||
| self.ops: Ops | None = None |
There was a problem hiding this comment.
Bar union types are a py 3.10 feature.
It only works here because the whole hint is stringified.
If someone wanted to introspect the types, or generate docs using py 3.8, that type hint won't get parsed, would it?
In fact, if we ran a type checker in py 3.8 mode, that wouldn't work either, which may be problematic, as we won't notice if some py 3.9+ feature is used accidentally.
I don't know if we're OK with that.
There was a problem hiding this comment.
If someone wanted to introspect the types, or generate docs using py 3.8, that type hint won't get parsed, would it?
The docs already need 3.10+ (because of the dependency on alabaster, which I assume is coming from the starter pack). I also feel like we don't need to support building the docs with the full range of Python versions that are supported for actually running charm tests. This change was actually motivated primarily to make the docs better.
What sort of type introspection are we expecting people to do? I think we're providing the type annotations to allow static type checking, not anything where you have tests doing different things at runtime based on the type.
In fact, if we ran a type checker in py 3.8 mode, that wouldn't work either, which may be problematic, as we won't notice if some py 3.9+ feature is used accidentally.
Are you sure? It seems to work in my testing. With this test code:
import ops
import scenario
ctx = scenario.Context(ops.CharmBase, meta={'name': 'foo', 'requires': {'db': {'interface': 'x'}}})
rel = scenario.Relation('db')
ctx.run(ctx.on.relation_joined(rel, remote_unit=3), scenario.State())I get no errors, even though this is using a | for union for remote_unit.
$ /tmp/sfutureenv/bin/pyright --pythonversion=3.8 /tmp/s-future.py
0 errors, 0 warnings, 0 informations
And if I change the int to a string, then I properly get the error:
$ /tmp/sfutureenv/bin/pyright --pythonversion=3.8 /tmp/s-future.py
/tmp/s-future.py
/tmp/s-future.py:6:49 - error: Argument of type "Literal['3']" cannot be assigned to parameter "remote_unit" of type "int | None" in function "relation_joined"
Type "Literal['3']" is not assignable to type "int | None"
"Literal['3']" is not assignable to "int"
"Literal['3']" is not assignable to "None" (reportArgumentType)
1 error, 0 warnings, 0 informations
This is using Python 3.8.18 with the branch version of scenario, and pyright 1.1.383.
If I use something newer than 3.8, like typing.LiteralString, then I get an error if I run tox using Python 3.8:
$ uvx --python=3.8 tox -e static
static: commands[0]> pyright --pythonversion=3.8 scenario
WARNING: there is a new pyright version available (v1.1.347 -> v1.1.383).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`
/home/tameyer/code/ops-scenario/scenario/state.py
/home/tameyer/code/ops-scenario/scenario/state.py:57:15 - error: "LiteralString" is not a known member of module "typing" (reportGeneralTypeIssues)
1 error, 0 warnings, 0 informations
static: exit 1 (2.03 seconds) /home/tameyer/code/ops-scenario> pyright --pythonversion=3.8 scenario pid=2061783
static: FAIL code 1 (2.06=setup[0.02]+cmd[2.03] seconds)
evaluation failed :( (2.09 seconds)
But also if I use 3.12 with --pythonversion=3.8:
$ uvx --python=3.12 tox -e static
Installed 11 packages in 4ms
static: recreate env because python changed version_info=[3, 8, 19, 'final', 0]->[3, 12, 3, 'final', 0] | executable='/home/tameyer/.local/share/uv/python/cpython-3.8.19-linux-x86_64-gnu/bin/python3.8'->'/usr/bin/python3.12'
static: remove tox env folder /home/tameyer/code/ops-scenario/.tox/static
static: install_deps> python -I -m pip install 'ops~=2.15' pyright==1.1.347
static: commands[0]> pyright --pythonversion=3.8 scenario
WARNING: there is a new pyright version available (v1.1.347 -> v1.1.383).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`
/home/tameyer/code/ops-scenario/scenario/state.py
/home/tameyer/code/ops-scenario/scenario/state.py:57:15 - error: "LiteralString" is not a known member of module "typing" (reportGeneralTypeIssues)
1 error, 0 warnings, 0 informations
static: exit 1 (1.93 seconds) /home/tameyer/code/ops-scenario> pyright --pythonversion=3.8 scenario pid=2067230
static: FAIL code 1 (2.97=setup[1.04]+cmd[1.93] seconds)
evaluation failed :( (3.06 seconds)
Do you have an example that fails to get detected?
The goal here is to reduce the use of the name "Scenario" in the docs, to control where it appears when the docs are included at ops.readthedocs.io. Using `from __future__ import annotations` seems to make Sphinx much happier when doing the class signatures, avoiding odd text like `~scenario.state.CloudCredential` instead of the expected link with text "CloudCredential" and destination that class. When adding those imports, pyupgrade transformed all the type annotations. I've run all the tests with 3.8, 3.9, 3.10, 3.11, and 3.12 and they all pass, and I've manually tested in 3.8 and everything seems to work without any problems. I like this much more, so it seems like a win-win. You may find it easier to review commit-by-commit, as [one commit is completely automated](7f250a1), running `pyupgrade`, and it is responsible for the majority of lines changed. Also removes a couple of references to the name "Scenario", as the original intent of this change was to clean up references in the docs. Migrated from canonical/ops-scenario#203
The goal here is to reduce the use of the name "Scenario" in the docs, to control where it appears when the docs are included at ops.readthedocs.io.
Using
from __future__ import annotationsseems to make Sphinx much happier when doing the class signatures, avoiding odd text like~scenario.state.CloudCredentialinstead of the expected link with text "CloudCredential" and destination that class.When adding those imports, pyupgrade transformed all the type annotations - I wasn't previously aware that the future import made this possible in 3.8, but it seems to be the case. I've run all the tests with 3.8, 3.9, 3.10, 3.11, and 3.12 and they all pass, and I've manually tested in 3.8 and everything seems to work without any problems. I like this much more, so it seems like a win-win.
(To be clear: that means most of the changes in this PR are automatic via pyupgrade).
The pyupgrade pre-commit hook was running twice, one with no version restrictions for a specific file that doesn't seem to exist any more, so I've removed the duplication. I've also changed pyupgrade and pyright to target 3.8 compatibility.
A couple of docstrings are changed to use the
State()style rather thanscenario.State()style.