sdk/python: use typing.List in classes that define a list method - #1477
sdk/python: use typing.List in classes that define a list method#1477dwin-gharibi wants to merge 1 commit into
list method#1477Conversation
…shadows builtin Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| created_at: str = "" | ||
| finished_at: str = "" | ||
| logs: list[str] = field(default_factory=list) | ||
| logs: List[str] = field(default_factory=list) |
There was a problem hiding this comment.
This rewrite isn't needed: TemplateBuild (like TemplateInfo) doesn't define a list method, so list[str] here already resolves to the builtin for both mypy and runtime. Only annotations lexically inside Template (which defines list) were affected by the shadowing — the same applies to replicas (line 83) and builds (line 87), which could stay list[...].
Reverting these three keeps the diff focused on the actual defect and consistent with the PEP-585 list[...] style used elsewhere in the same files (_models.py, _commands.py). It also avoids introducing typing.List in scopes where the builtin isn't shadowed, which ruff's UP006 (enabled via select = ["E", "F", "I", "UP"] in sdk/python/pyproject.toml) would flag if a full ruff check is ever run. (Not blocking — the changes are harmless.)
|
|
||
|
|
||
| def _serialize_volume_mounts(mounts: VolumeMountsArg) -> list[dict[str, object]]: | ||
| def _serialize_volume_mounts(mounts: VolumeMountsArg) -> List[dict[str, object]]: |
There was a problem hiding this comment.
_serialize_volume_mounts is a module-level function, so list[dict[str, object]] here (and the serialized local on line 203) was never affected by the Volume.list shadowing — mypy resolves list to the builtin at module scope. The only annotation in this file that actually needed the fix is the return type of Volume.list (line 379). Consider reverting lines 201 and 203 to list[...] to keep the diff minimal and consistent with the surrounding PEP-585 style.
Review: sdk/python: use
|
|
Thank you for your PR. Could you please provide more details about the test results? |
Closes #1476.
Motivation
Template,Sandbox,VolumeandFilesystemeach define a method namedlist. Inside a class body thename
listbinds to that method, so an annotation writtenlist[TemplateInfo]in the same class refers tothe method, not the builtin. mypy reports
Function "...list" is not valid as a typeand, more importantly,stops type-checking those parameters — 10 annotations across two files were effectively
Any.What this changes
Replaces
list[...]withtyping.List[...]in the four affected modules and addsListto eachfrom typing import ...line:cubesandbox/_template.pycubesandbox/sandbox.pycubesandbox/_volume.pycubesandbox/_filesystem.pytyping.Listis not shadowed by anything, so the annotations resolve for static analysers as well as atruntime. The public API — including the
listmethod names — is unchanged.I fixed all four rather than only the two mypy currently flags:
_volume.pyand_filesystem.pyhave thesame construct and would start producing the same errors as soon as anything about their annotations
changed.
No comment changes.
Correcting the original report
The issue I filed first claimed this broke
typing.get_type_hintsat runtime. That was wrong, and Iverified it rather than leaving it in:
All four modules use
from __future__ import annotations, andget_type_hintsevaluates against moduleglobals rather than the class namespace, so
listresolves to the builtin at runtime. The defect isstatic-analysis only, and the issue text has been corrected to say so. That also lowers the severity from
what I first suggested.
Testing
The evidence is the mypy count, since the defect is a type-resolution one:
No behavioural change, so no new tests. Existing suite and lint are unaffected:
CI gates checked locally:
pytest— 225 passed (sdk-test-check).ruff— no new findings (the twoF401unused-import errors are present on master too and are nottouched here).