Misc typing fixes in not annotated functions #6389

marcospri · 2024-06-25T08:30:14Z

Quick fixes with the goal of enabling mypy's check_untyped_defs

First round of fixes.

marcospri · 2024-06-25T08:33:05Z

lms/config.py

@@ -39,7 +39,7 @@ class _Setting:
    """The properties of a setting and how to read it."""

    name: str
-    read_from: str | None = None
+    read_from: str = None  # type: ignore


We use this pattern, allowing None in a dataclass field to provide a default in __post_init__

Ignore the type here to avoid multiple issues about it being nullable all over the place.

We should probably just remove the read_from feature, it's only used once

lms/extensions/feature_flags/__init__.py

marcospri · 2024-06-25T08:34:17Z

lms/models/application_instance.py

@@ -92,7 +92,7 @@ class ApplicationInstance(CreatedUpdatedMixin, Base):

    consumer_key = sa.Column(sa.Unicode, unique=True, nullable=True)
    shared_secret = sa.Column(sa.Unicode, nullable=False)
-    lms_url = sa.Column(sa.Unicode(2048), nullable=False)
+    lms_url: Mapped[str] = mapped_column(sa.Unicode(2048), nullable=False)


We could probably automate a conversion to this style for all models. Doing only the ones that solve issues in this PR.

I think nullable=False is the default when using Mapped[str]. (If you want it to be nullable you have to do Mapped[str | None])

marcospri · 2024-06-25T08:35:47Z

lms/services/canvas.py

 from lms.services.exceptions import CanvasAPIPermissionError, FileNotFoundInCourse


 class CanvasService:
    """A high level Canvas service."""

-    api = None
+    api: CanvasAPIClient = None  # type:ignore


AFAICT this is only here for the tests benefict. Ignore this declaration so all the code below assumes the right type.

marcospri · 2024-06-25T08:36:19Z

lms/services/organization.py

@@ -43,7 +43,7 @@ def get_by_id(self, id_: int) -> Organization | None:

        return self._organization_search_query(id_=id_).one_or_none()

-    def get_by_public_id(self, public_id: str) -> list | None:
+    def get_by_public_id(self, public_id: str) -> Organization | None:


This was wrong

seanh

I did a quick test of dumping the DB schema from a DB created on main and from one created on this branch diffing them, the only difference is the change in the type of the deployment_id column which I noted. Probably a good idea to do this test whenever changing the models

seanh · 2024-06-25T09:52:04Z

lms/config.py

@@ -39,7 +39,7 @@ class _Setting:
    """The properties of a setting and how to read it."""

    name: str
-    read_from: str | None = None
+    read_from: str = None  # type: ignore


We should probably just remove the read_from feature, it's only used once

seanh · 2024-06-25T09:55:26Z

lms/db/_columns.py

    """Return a SA column type to store the python enum.Enum as a varchar in a table."""
-    return sa.Column(
+    return mapped_column(


This function is probably a pretty good example of SQLAlchemy cleverness that we'd be better off without. If we want to add Python-only validation to a column, SQLAlchemy has a simple way to do that: https://docs.sqlalchemy.org/en/20/orm/mapped_attributes.html#simple-validators

seanh · 2024-06-25T09:56:24Z

lms/models/application_instance.py

@@ -92,7 +92,7 @@ class ApplicationInstance(CreatedUpdatedMixin, Base):

    consumer_key = sa.Column(sa.Unicode, unique=True, nullable=True)
    shared_secret = sa.Column(sa.Unicode, nullable=False)
-    lms_url = sa.Column(sa.Unicode(2048), nullable=False)
+    lms_url: Mapped[str] = mapped_column(sa.Unicode(2048), nullable=False)


I think nullable=False is the default when using Mapped[str]. (If you want it to be nullable you have to do Mapped[str | None])

seanh · 2024-06-25T09:59:40Z

lms/models/lti_role.py

@@ -39,27 +40,25 @@ class LTIRoleOverride(Base):

    id = sa.Column(sa.Integer(), autoincrement=True, primary_key=True)

-    lti_role_id = sa.Column(
-        sa.Integer(),
+    lti_role_id: Mapped[int] = mapped_column(
        sa.ForeignKey("lti_role.id", ondelete="cascade"),
        nullable=True,


If this has nullable=True shouldn't it be Mapped[int | None]? (Then you wouldn't need the nullable=True)

seanh · 2024-06-25T10:00:10Z

lms/models/lti_role.py


-    application_instance_id = sa.Column(
-        sa.Integer(),
+    application_instance_id: Mapped[int] = mapped_column(
        sa.ForeignKey("application_instances.id", ondelete="cascade"),
        nullable=False,


I think nullable=False is the default with Mapped

seanh · 2024-06-25T10:13:13Z

lms/models/application_instance.py

    )

    lti_registration = sa.orm.relationship(
        "LTIRegistration", back_populates="application_instances"
    )

    # Unique identifier of this instance per LTIRegistration
-    deployment_id = sa.Column(sa.UnicodeText, nullable=True)
+    deployment_id: Mapped[str | None] = mapped_column()


This changes the type of the column from text to character varying

Quick fixes with the goal of enabling mypy's check_untyped_defs

…bled

marcospri commented Jun 25, 2024

View reviewed changes

marcospri force-pushed the mypy-functions branch from b92c019 to 678c29a Compare June 25, 2024 08:44

marcospri requested a review from seanh June 25, 2024 08:47

seanh approved these changes Jun 25, 2024

View reviewed changes

marcospri added 2 commits June 26, 2024 14:45

Misc typing fixes in not annotated functions

6ab0e7f

Quick fixes with the goal of enabling mypy's check_untyped_defs

Remove ignores that don't take effect until check_untyped_defs is ena…

ebbe824

…bled

marcospri force-pushed the mypy-functions branch from 678c29a to ebbe824 Compare June 26, 2024 12:45

marcospri merged commit f9c8246 into main Jun 26, 2024
9 checks passed

marcospri deleted the mypy-functions branch June 26, 2024 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc typing fixes in not annotated functions #6389

Misc typing fixes in not annotated functions #6389

marcospri commented Jun 25, 2024 •

edited

Loading

marcospri Jun 25, 2024

seanh Jun 25, 2024

marcospri Jun 25, 2024

seanh Jun 25, 2024

marcospri Jun 25, 2024 •

edited

Loading

marcospri Jun 25, 2024

seanh left a comment

seanh Jun 25, 2024

seanh Jun 25, 2024

seanh Jun 25, 2024

seanh Jun 25, 2024

seanh Jun 25, 2024

seanh Jun 25, 2024

Misc typing fixes in not annotated functions #6389

Misc typing fixes in not annotated functions #6389

Conversation

marcospri commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri commented Jun 25, 2024 •

edited

Loading

marcospri Jun 25, 2024 •

edited

Loading