agdsn / pycroft

The AG DSN management system
Apache License 2.0
19 stars 9 forks source link

Automated static type checking with MyPy #559

Open lukasjuhrich opened 2 years ago

lukasjuhrich commented 2 years ago

Many bugs in the past could have been avoided by some simple type checking. This is a Meta-Issue tracking the progress and to use as a reference in commits which extend our typing efforts.

Sub-Issues / milestones

Acceptance criteria

There is a mypy CI checking at least pycroft and web (with --check-untyped-defs).

lukasjuhrich commented 2 years ago

Just because I don't know where else to write this, the following is worth noting about how to type the SQLAlchemy models:

Above, the ultimate class-level datatypes of id, name and other_name will be introspected as Mapped[Optional[int]], Mapped[Optional[str]] and Mapped[Optional[str]]. The types are by default always considered to be Optional, even for the primary key and non-nullable column. The reason is because while the database columns “id” and “name” can’t be NULL, the Python attributes id and name most certainly can be None without an explicit constructor:

(from the docs)

lukasjuhrich commented 2 years ago

So, fundamentally, SQLAlchemy/mypy is not able to distinguish between

  1. a newly constructed User
  2. a User fetched from a database.

This discrepancy is what statically typed languages usually avoid with the builder pattern.

lukasjuhrich commented 2 years ago

Also relevant: https://mypy.readthedocs.io/en/stable/common_issues.html

lukasjuhrich commented 1 year ago

See also these notes

lukasjuhrich commented 1 year ago

web enabled by #661