Adding ty type checking (experimental) - #1371
Conversation
|
Sounds like we should introduce this gradually, so we can have it run on modules that pass for first pass and then be able to merge. |
|
To get this to pass, install something like types-requests. |
| error_content = {str(key): value for key, value in json_content.items()} | ||
| detail = error_content.get("detail") | ||
| alternative = detail if isinstance(detail, str) else "" | ||
| message_content = error_content.get("message") | ||
| message = message_content if isinstance(message_content, str) else alternative |
There was a problem hiding this comment.
if we change the annotation of _handle_response to pydantic.JsonValue (which is more precise anyway) we can revert these changes
There was a problem hiding this comment.
I agree with updating annotation of _handle_response to pydantic.JsonValue and while this satisfies ty it introduces an error from mypy instead (since json_content.get("detail", "") is not garunteed to be a string.
Ultimately, either way some handling of non-string response values is required.
I've modified a little bit to now satisfy both
| return credentials | ||
|
|
||
| @staticmethod | ||
| def _normalize_job_ids(job_ids: Sequence[str] | Sequence[uuid.UUID]) -> list[uuid.UUID]: |
There was a problem hiding this comment.
pydantic is supposed to do this kind of type coercion for us, i don't think we should be adding these manual validations/conversions on top of it (for example the pydantic plugin for mypy tells it to treat model arguments as Any by default, but unfortunately it seems like ty's pydantic support is still a wip..)
not sure how to best circumvent this though.. one possible (but definitely suboptimal) workaround would be to add something like
[tool.ty.analysis]
replace-imports-with-any = ["pydantic.main.**"]
to our pyproject.yaml?
There was a problem hiding this comment.
Pydantic does the coercion for us, but ty is correct that we are technically passing in a value of the wrong type since pydantic doesnt have a way of annotating the coercion it does. For now my suggestion is targeted ignore lines.
There was a problem hiding this comment.
hmm now i'm a little confused. shouldn't the type checker only care about the annotation of __init__ when checking the arguments passed to the class constructor? pydantic.BaseModel.__init__ has this annotation:
def __init__(self, /, **data: Any) -> None:which should mean that all input arguments accept anything
| @@ -93,6 +93,27 @@ jobs: | |||
| run: | | |||
| checks/mypy_.py | |||
|
|
|||
| ty: | |||
| name: Type check (ty) | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - uses: actions/checkout@v6 | |||
| - name: Set up Python 3.14 | |||
| uses: actions/setup-python@v6 | |||
| with: | |||
| python-version: 3.14 | |||
| - uses: actions/cache@v5 | |||
| with: | |||
| path: ${{ env.pythonLocation }} | |||
| key: ${{ env.pythonLocation }}-${{ hashFiles('**/pyproject.toml', '**/*requirements.txt', '.github/workflows/ci.yml') }} | |||
| - name: Install dependencies | |||
| run: | | |||
| python -m pip install --upgrade pip | |||
| pip install -e ./checks-superstaq -e ./general-superstaq[dev] -e ./qiskit-superstaq[dev] -e ./cirq-superstaq[dev] -e ./supermarq-benchmarks[dev] | |||
| - name: Type check | |||
| run: | | |||
| checks/ty_.py general-superstaq | |||
|
|
|||
| coverage: | |||
| name: Pytest and Coverage check | |||
| strategy: | |||
There was a problem hiding this comment.
Do we set a minimum supported Python version for client? We say 3.9 in the docs. Or, is the goal to bring all type-syntax up to date with 3.14?
There was a problem hiding this comment.
Relevant: ty docs on inferring the Python version.
We may want to check explicitly setting this (like you can in mypy)
| parser.description = textwrap.dedent( | ||
| """ | ||
| Runs ty on the repository (typing check). | ||
| Ignores files in the [repo_root]/examples directory. |
There was a problem hiding this comment.
Is this handled somewhere? Or are we ignoring it for now since gss has no examples?
There was a problem hiding this comment.
Nope, that was because I copied it from somewhere else - thanks for catching!
|
Dev discussion: Consider doing some kind of |
|
Oh yeah, pyrefly looks interesting. I read a useful comparison blog which highlights some of the differences in approach nicely: https://blog.edward-li.com/tech/comparing-pyrefly-vs-ty/ |
No description provided.