Moar types

[richardebeling]

because moar = better

I think we can conclude by now that python type hints are not going anywhere, so we may just as well annotate our interfaces to benefit from better LSP understanding and type checking.

Split into two commits, the first one should be very quick to review, containing only type changes. The second one contains everything that needs to touch actual production logic, mostly by adding assertions.

[niklasmohrin]

some more files checked off; should we maybe split this into multiple PRs so that reviewed changes can get merged before they get conflicts?

[niklasmohrin]

This is pretty wild, why do we annotate instance just to cast it to Any, and shouldn't source be Evaluation.State? We use it like it is in state_changed_to above

edit: I think this is a bug above 🤔

[richardebeling]

My thought was that instance is an evaluation, so I think we should annotate it as such, independently of what we do with it. The fact that we monkey-annotate some attribute that isn't part of the static "class interface" is orthogonal (-> hence cast to Any). I don't care much about this one-liner here, though, so I can just change it back to Any.

Regarding source/"bug above": In all our tests, this is only ever called with an Evaluation.State value, I'll narrow the type. Would that solve the issue you saw here, or is there still something open then?

Edit: Never mind, we have tests where it is called with a source: int value. I think this is not immediately a problem, since Evaluation.State is a IntegerChoices instance, which I guess can be used somewhat interchangeably with raw ints?

>>> int(Evaluation.State.NEW)
10
>>> 6 == Evaluation.State.NEW
False
>>> 10 == Evaluation.State.NEW
True
>>> 10 in [Evaluation.State.NEW]
True

[niklasmohrin]

okay fair enough; I feel that we should promote to our enum as early as possible at runtime, but as you show it's not broken so no concern for this PR

[niklasmohrin]

(todo: continue reading from here)

[niklasmohrin]

Suggested change

	match cast("InstanceActionType", self.action_type):
	match InstanceActionType[self.action_type]:

[richardebeling]

We have the underlying string value in self.action_type (here: "create", "change", "delete"), but this construction syntax only accepts the python enum element names ("CREATE", "CHANGE", "DELETE").

>>> from evap.evaluation.models_logging import InstanceActionType
>>> InstanceActionType["create"]
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/nix/store/829wb290i87wngxlh404klwxql5v18p4-python3-3.13.7/lib/python3.13/enum.py", line 793, in __getitem__
    return cls._member_map_[name]
           ~~~~~~~~~~~~~~~~^^^^^^
KeyError: 'create'
>>> InstanceActionType["CREATE"]
<InstanceActionType.CREATE: 'create'>

So with this change, we get test failures

[niklasmohrin]

ah, the typechecker fooled me into believing this works; what we want is this?

Suggested change

	match cast("InstanceActionType", self.action_type):
	match InstanceActionType(self.action_type):

[niklasmohrin]

if you change

1. the class def to class ExcelFileRowMapper[R: InputRow]
2. InputRow.from_cells to return Self (from typing)

then we can return list[R] here

[richardebeling]

Hmm, with diff

diff --git evap/staff/importers/base.py evap/staff/importers/base.py
index 632b6f7e..bbb187bb 100644
--- evap/staff/importers/base.py
+++ evap/staff/importers/base.py
@@ -5,7 +5,7 @@ from collections.abc import Iterable, Iterator
 from dataclasses import dataclass
 from enum import Enum
 from io import BytesIO
-from typing import Any
+from typing import Any, Self
 
 import openpyxl
 from django.conf import settings
@@ -191,11 +191,11 @@ class InputRow(ABC):
         pass
 
     @classmethod
-    def from_cells(cls, location: ExcelFileLocation, cells: Iterable[str]) -> "InputRow":
+    def from_cells(cls, location: ExcelFileLocation, cells: Iterable[str]) -> Self:
         return cls(location, *cells)
 
 
-class ExcelFileRowMapper:
+class ExcelFileRowMapper[R: InputRow]:
     """
     Take a excel file and map it to a list of row_cls instances
     """
@@ -205,7 +205,7 @@ class ExcelFileRowMapper:
         self.row_cls = row_cls
         self.importer_log = importer_log
 
-    def map(self, file_content: bytes) -> list:
+    def map(self, file_content: bytes) -> list[R]:
         try:
             book = openpyxl.load_workbook(BytesIO(file_content))
         except Exception as e:  # noqa: BLE001

I get

 $ ./manage.py typecheck
Executing mypy
evap/staff/importers/base.py:257: error: Incompatible return value type (got "list[InputRow]", expected "list[R]")  [return-value]
evap/staff/importers/user.py:328: error: Need type annotation for "excel_mapper"  [var-annotated]
evap/staff/importers/enrollment.py:707: error: Need type annotation for "input_rows" (hint: "input_rows: list[<type>] = ...")  [var-annotated]
Found 3 errors in 3 files (checked 508 source files)

---

This diff works, but it's still somewhat ugly because we now pass the class two times, once for the type system and once to be used as a run-time value. Do we want that?

diff --git evap/staff/importers/base.py evap/staff/importers/base.py
index 632b6f7e..ed5a4297 100644
--- evap/staff/importers/base.py
+++ evap/staff/importers/base.py
@@ -5,7 +5,7 @@ from collections.abc import Iterable, Iterator
 from dataclasses import dataclass
 from enum import Enum
 from io import BytesIO
-from typing import Any
+from typing import Any, Self
 
 import openpyxl
 from django.conf import settings
@@ -191,21 +191,21 @@ class InputRow(ABC):
         pass
 
     @classmethod
-    def from_cells(cls, location: ExcelFileLocation, cells: Iterable[str]) -> "InputRow":
+    def from_cells(cls, location: ExcelFileLocation, cells: Iterable[str]) -> Self:
         return cls(location, *cells)
 
 
-class ExcelFileRowMapper:
+class ExcelFileRowMapper[R: InputRow]:
     """
     Take a excel file and map it to a list of row_cls instances
     """
 
-    def __init__(self, skip_first_n_rows: int, row_cls: type[InputRow], importer_log: ImporterLog):
+    def __init__(self, skip_first_n_rows: int, row_cls: type[R], importer_log: ImporterLog):
         self.skip_first_n_rows = skip_first_n_rows
         self.row_cls = row_cls
         self.importer_log = importer_log
 
-    def map(self, file_content: bytes) -> list:
+    def map(self, file_content: bytes) -> list[R]:
         try:
             book = openpyxl.load_workbook(BytesIO(file_content))
         except Exception as e:  # noqa: BLE001
diff --git evap/staff/importers/enrollment.py evap/staff/importers/enrollment.py
index a36cb5f5..36a66e8e 100644
--- evap/staff/importers/enrollment.py
+++ evap/staff/importers/enrollment.py
@@ -704,7 +704,7 @@ def import_enrollments(
     importer_log = ImporterLog()
 
     with ConvertExceptionsToMessages(importer_log):
-        input_rows = ExcelFileRowMapper(
+        input_rows = ExcelFileRowMapper[EnrollmentInputRow](
             skip_first_n_rows=1,
             row_cls=EnrollmentInputRow,
             importer_log=importer_log,
diff --git evap/staff/importers/user.py evap/staff/importers/user.py
index 51f82e99..fd3f60d1 100644
--- evap/staff/importers/user.py
+++ evap/staff/importers/user.py
@@ -325,7 +325,7 @@ def import_users(excel_content: bytes, test_run: bool) -> tuple[list[UserProfile
     importer_log = ImporterLog()
 
     with ConvertExceptionsToMessages(importer_log):
-        excel_mapper = ExcelFileRowMapper(skip_first_n_rows=1, row_cls=UserInputRow, importer_log=importer_log)
+        excel_mapper = ExcelFileRowMapper[UserInputRow](skip_first_n_rows=1, row_cls=UserInputRow, importer_log=importer_log)
         raw_rows = excel_mapper.map(excel_content)
         importer_log.raise_if_has_errors()

[niklasmohrin]

I would expect that the class-wide type would be inferred if you pass row_cls=UserInputRow / I thought that it did infer it when I tried it out; I also like the second diff as-is if it's not inferred