Skip to content

add type hints #1343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

add type hints #1343

wants to merge 9 commits into from

Conversation

prandla
Copy link
Contributor

@prandla prandla commented May 28, 2025

Converting old docstring-based type hints to PEP 484 type hints. Current progress: all of cms/ except for cms/server/, and all of cmscommon/.

Notes:

  • SQLAlchemy models are typed mostly correctly (required a few hacks, see diff of cms/db/base.py). SQLAlchemy queries are still untyped, requires upgrading to SQLAlchemy 2.0. It's also possible to manually type-hint the result of queries though.
  • RPC method calls aren't type-checked. I couldn't figure out a way to do that without changing the method call syntax; I think it would be possible if RPC calls looked like other_service.rpc.method(args) (as opposed to the current other_service.method(args)).
  • Minimum Python version is now 3.12 (for the new generic syntax). It would be pretty easy to port to python 3.11, but i don't want to go below that. I think this is not a huge issue, most distros have >=3.12 already, Debian is the last holdout and a new debian release should be happening in a few months.
  • all of our jinja templates will forever be non-type-checked. this makes me sad :(

Converting old docstring-based type hints to PEP 484 type hints. This
commit includes all of cms/ except for cms/server/, and all of cmscommon/.
@veluca93
Copy link
Collaborator

@gollux is the python 3.12 minimum version requirement likely to be a problem for IOI?

@gollux
Copy link
Contributor

gollux commented May 28, 2025

This IOI will run on Ubuntu 2024.04.02 with Python 3.12.3.

However, I would prefer requiring at least 3.11, because that's in stable Debian, which is sort of the lower bound for all reasonable Linux distros ;)

What feature from 3.12 do you need?

@prandla
Copy link
Contributor Author

prandla commented May 28, 2025

What feature from 3.12 do you need?

the new type parameter syntax. i also wanted to use typing.override, but couldn't be bothered with it (there's a few functions where the return type depends on a boolean argument, using override allows typing them more precisely than a simple union).

(edit: brain fart, i meant overload, which was introduced before 3.12 and is actually fine to use)

it's okay, i'll just replace it with manual typing.TypeVar usage.

we might want to also run tests on the oldest version of python we want to support.

@prandla
Copy link
Contributor Author

prandla commented May 28, 2025

Tests now pass on python 3.11 aswell. I'll make a separate PR later to automatically test this on github actions, for now i'm testing on 3.11 locally.

prandla added 7 commits May 29, 2025 15:24
Added type hints to cms/server and fixed some previous type hint
mistakes.
guard all tornado imports behind `if typing.TYPE_CHECKING`. i think this
allows the "tornado4" workaround to still work, though i don't know if
it's relevant anymore.
@prandla prandla marked this pull request as ready for review May 31, 2025 15:08
@prandla
Copy link
Contributor Author

prandla commented May 31, 2025

some more notes:

for cmsranking and cmscontrib i tried to add type hints to all functions, even where they weren't present before. for cmstestsuite i only converted the old type hints to new-style ones.

there's a few modules where it would be possible to have much better static typing if the code was refactored a bit. currently i tried to keep the existing code untouched. (though i added a few asserts that should always be true, in cases where there's some complicated control flow and the type checker can't figure out that something is always not-None.)

the web server parts had less type hints than i expected; this is mostly because a lot of it is just passing parameters into jinja templates which are, again, completely untyped.

A lot of the remaining type errors are about sqlalchemy queries, which could be fixed by upgrading to sqlalchemy 2.0 (i think). I might try that at some point. The rest are in tornado/werkzeug/jinja, and as i understand, upgrading those is quite the ordeal. But I suppose it must still happen some day. And there's also a lot of type errors about missing None checks, most of them false positives, some of them quite certainly real :)

@prandla prandla changed the title [WIP] add type hints add type hints May 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants