Miscellaneous python typing fixes #6122

marcospri · 2024-03-14T14:24:50Z

The python typecheker (mypy) can be run with make typecheck. It's not enabled in either make sure or in CI.

Fix some low-hanging-fruit issues and add a few ignores in more complicated ones.

marcospri · 2024-03-14T14:25:29Z

lms/models/application_instance.py

@@ -80,7 +80,7 @@ class ApplicationInstance(CreatedUpdatedMixin, Base):
    organization_id = sa.Column(
        sa.Integer(), sa.ForeignKey("organization.id"), nullable=True
    )
-    organization = sa.orm.relationship("Organization")
+    organization: sa.Mapped["Organization"] = sa.orm.relationship("Organization")


We'll eventually should type all the models as they are at the core of the codebase.

marcospri · 2024-03-14T14:26:06Z

lms/product/plugin/plugin.py

-    grouping: GroupingPlugin = _LazyPlugin()
-    course_copy: CourseCopyPlugin = _LazyPlugin()
-    misc: MiscPlugin = _LazyPlugin()
+    grouping: GroupingPlugin = _LazyPlugin()  # type: ignore


This code it's a bit magical. Not worth trying to fix this.

Should the comment be # type:ignore?

Suggested change

grouping: GroupingPlugin = _LazyPlugin() # type: ignore

grouping: GroupingPlugin = _LazyPlugin() # type:ignore

# type: ignore is canonical.

Using # type: ignore here works fine as mypy does still use the annotated type (GroupingPlugin) in other contexts. Sometimes in TypeScript silencing type errors can cause unexpected issues in code which uses the result of the expression where the check was suppressed. An alternative approach is to use typing.cast:

diff --git a/lms/product/plugin/plugin.py b/lms/product/plugin/plugin.py index 66dcfea02..44cdf2a07 100644 --- a/lms/product/plugin/plugin.py +++ b/lms/product/plugin/plugin.py @@ -1,4 +1,5 @@ from dataclasses import dataclass +from typing import cast from lms.product.plugin.course_copy import CourseCopyPlugin from lms.product.plugin.grouping import GroupingPlugin @@ -31,9 +32,9 @@ class Plugins: setattr(instance, self.plugin_name, plugin) # Overwrite the attr return plugin - grouping: GroupingPlugin = _LazyPlugin() # type: ignore - course_copy: CourseCopyPlugin = _LazyPlugin() # type:ignore - misc: MiscPlugin = _LazyPlugin() # type:ignore + grouping = cast(GroupingPlugin, _LazyPlugin()) + course_copy = cast(CourseCopyPlugin, _LazyPlugin()) + misc = cast(MiscPlugin, _LazyPlugin()) def __init__(self, request, plugin_config: PluginConfig): self._request = request

Went for the type casting 👍

marcospri · 2024-03-14T14:26:18Z

lms/services/canvas_studio.py

@@ -219,7 +219,7 @@ def list_collection(self, collection_id: str) -> list[File]:

        return files

-    def _api_request(self, path: str, schema_cls: RequestsResponseSchema) -> dict:
+    def _api_request(self, path: str, schema_cls: Type[RequestsResponseSchema]) -> dict:


Object type vs passing a class.

lms/services/http.py

marcospri · 2024-03-14T14:26:51Z

lms/services/upsert.py

@@ -45,12 +45,12 @@ def bulk_upsert(

    index_elements_columns = [getattr(model_class, c) for c in index_elements]

-    stmt = insert(model_class).values(values)
-    stmt = stmt.on_conflict_do_update(
+    base = insert(model_class).values(values)


These two had different types, use different names.

marcospri · 2024-03-14T14:27:27Z

lms/services/vitalsource/service.py

@@ -92,9 +92,9 @@ def get_document_url(
        url = urlparse(url)
        params = parse_qs(url.query)
        if end_page:
-            params["end_page"] = end_page
+            params["end_page"] = end_page  # type: ignore


parse_qs seems to return dict[str, list[str]]

marcospri · 2024-03-14T14:28:22Z

lms/validation/_base.py

    """
    Whether or not this schema validates collections of objects by default.

-    If this is ``None`` then marshmallow's default behavior will be used -- the
+    If this is ``False`` then marshmallow's default behavior will be used -- the


https://github.com/marshmallow-code/marshmallow/blob/dev/src/marshmallow/schema.py#L369

marcospri · 2024-03-14T14:28:45Z

pyproject.toml

@@ -147,7 +147,6 @@ python_version = 3.11

 disable_error_code = [
    "arg-type",
-    "assignment",


The plan is to remove codes from here one by one.

marcospri · 2024-03-14T14:37:33Z

lms/services/http.py

@@ -7,7 +7,7 @@ class HTTPService:
    """Send HTTP requests with `requests` and receive the responses."""

    # This is here mostly to let auto-spec know about it in the tests
-    session: Session = None
+    session: Session = None  # typing: ignore


Removing the default makes the test fail, which is the reason to have this in the first place.

I had a look through the Python docs for create_autospec, which suggests some alternative ways of handling this. I include the diff below to illustrate, but # typing: ignore is simpler.

diff --git a/lms/services/http.py b/lms/services/http.py index 8d0e73fca..8c282b3c5 100644 --- a/lms/services/http.py +++ b/lms/services/http.py @@ -6,8 +6,7 @@ from lms.services.exceptions import ExternalRequestError class HTTPService: """Send HTTP requests with `requests` and receive the responses.""" - # This is here mostly to let auto-spec know about it in the tests - session: Session = None # typing: ignore + session: Session """The underlying requests Session.""" def __init__(self): @@ -15,7 +14,6 @@ class HTTPService: # requests and urllib3 connection pooling is used (which means that # underlying TCP connections are re-used when making multiple requests # to the same host, e.g. pagination). - # See https://docs.python-requests.org/en/latest/user/advanced/#session-objects self.session = Session() diff --git a/tests/unit/lms/services/jstor/service_test.py b/tests/unit/lms/services/jstor/service_test.py index d5152d0c3..ea4d86ad1 100644 --- a/tests/unit/lms/services/jstor/service_test.py +++ b/tests/unit/lms/services/jstor/service_test.py @@ -5,7 +5,7 @@ from unittest.mock import sentinel import pytest from lms.services import ExternalRequestError -from lms.services.jstor.service import ArticleNotFound, JSTORService +from lms.services.jstor.service import ArticleNotFound, HTTPService, JSTORService from tests import factories API_URL = "http://jstore_api.example.com" @@ -200,7 +200,12 @@ class TestJSTORService: @pytest.fixture(autouse=True) def HTTPService(self, patch): - return patch("lms.services.jstor.service.HTTPService") + class HTTPServiceForTest(HTTPService): + session = None + + return patch( + "lms.services.jstor.service.HTTPService", autospec=HTTPServiceForTest + ) @pytest.fixture(autouse=True) def JWTService(self, patch): diff --git a/tests/unit/lms/services/vitalsource/_client_test.py b/tests/unit/lms/services/vitalsource/_client_test.py index a0c43921e..9da609b36 100644 --- a/tests/unit/lms/services/vitalsource/_client_test.py +++ b/tests/unit/lms/services/vitalsource/_client_test.py @@ -7,6 +7,7 @@ from requests import Request from lms.services.exceptions import ExternalRequestError from lms.services.vitalsource._client import ( BookNotFound, + HTTPService, VitalSourceClient, VitalSourceConfigurationError, _VSUserAuth, @@ -253,9 +254,13 @@ class TestVitalSourceClient: @pytest.fixture(autouse=True) def http_service(self, patch): - HTTPService = patch("lms.services.vitalsource._client.HTTPService") + class HTTPServiceForTest(HTTPService): + session = None - return HTTPService.return_value + cls = patch( + "lms.services.vitalsource._client.HTTPService", autospec=HTTPServiceForTest + ) + return cls.return_value @pytest.fixture def _VSUserAuth(self, patch): diff --git a/tests/unit/services.py b/tests/unit/services.py index 6e71bc381..a43513842 100644 --- a/tests/unit/services.py +++ b/tests/unit/services.py @@ -1,6 +1,7 @@ from unittest import mock import pytest +from requests import Session from lms.product.plugin.course_copy import ( CourseCopyFilesHelper, @@ -234,9 +235,11 @@ def h_api(mock_service): @pytest.fixture def http_service(mock_service): - http_service = mock_service(HTTPService, service_name="http") - http_service.request.return_value = factories.requests.Response() + class HTTPServiceForTest(HTTPService): + session = mock.create_autospec(Session, spec_set=True, instance=True) + http_service = mock_service(HTTPServiceForTest, service_name="http") + http_service.request.return_value = factories.requests.Response() return http_service

Should we then define the type as Session | None?

marcospri · 2024-03-14T15:04:45Z

lms/product/blackboard/product.py



 @dataclass
 class Blackboard(Product):
    """A product for Blackboard specific settings and tweaks."""

-    family: Product.Family = Product.Family.BLACKBOARD


Access these more directly

robertknight

From the changes here the one thing I think we might want to look into a better pattern for is avoiding adding extra | Nones for dataclass fields which are initialized in __post_init__ since that will lead to unnecessary checks in code that handles constructed dataclass instances.

robertknight · 2024-03-15T08:14:08Z

lms/config.py

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

    name: str
-    read_from: str = None
-    value_mapper: Callable = None
+    read_from: str | None = None


Using str | None here has the downside that mypy will ask you to add checks that this is non-null whenever using an initialized instance. If I understand correctly, this value is only None during construction? There are some Stack Overflow posts discussing this issue, such as https://stackoverflow.com/q/74621969/434243.

I'll keep looking for a good solution in a different PR.

The feature we are after is for dataclass fields that can be used during init but post__init provides a default if that's missing.

robertknight · 2024-03-15T08:20:03Z

lms/models/json_settings.py

@@ -100,7 +100,7 @@ def set(self, group, key, value):
        # pylint:disable=unsupported-assignment-operation
        super().setdefault(group, {})[key] = value

-    def set_secret(self, aes_service, group, key, value: str):
+    def set_secret(self, aes_service, group, key, value: str) -> None:


It's unfortunate that there is no type inference for return types, even functions that return None. Hopefully this might happen in future.

robertknight · 2024-03-15T08:22:49Z

lms/models/public_id.py

@@ -28,7 +28,7 @@ class PublicId:
    app_code: str = "lms"
    """Code representing the product this model is in."""

-    instance_id: str = None
+    instance_id: str | None = None


Same comment as above about data class fields which are only null during initialization. It would be good to find a pattern so code which only handles initialized instances doesn't need unnecessary non-null checks.

robertknight · 2024-03-15T08:31:28Z

lms/product/plugin/plugin.py

-    grouping: GroupingPlugin = _LazyPlugin()
-    course_copy: CourseCopyPlugin = _LazyPlugin()
-    misc: MiscPlugin = _LazyPlugin()
+    grouping: GroupingPlugin = _LazyPlugin()  # type: ignore


# type: ignore is canonical.

Using # type: ignore here works fine as mypy does still use the annotated type (GroupingPlugin) in other contexts. Sometimes in TypeScript silencing type errors can cause unexpected issues in code which uses the result of the expression where the check was suppressed. An alternative approach is to use typing.cast:

diff --git a/lms/product/plugin/plugin.py b/lms/product/plugin/plugin.py index 66dcfea02..44cdf2a07 100644 --- a/lms/product/plugin/plugin.py +++ b/lms/product/plugin/plugin.py @@ -1,4 +1,5 @@ from dataclasses import dataclass +from typing import cast from lms.product.plugin.course_copy import CourseCopyPlugin from lms.product.plugin.grouping import GroupingPlugin @@ -31,9 +32,9 @@ class Plugins: setattr(instance, self.plugin_name, plugin) # Overwrite the attr return plugin - grouping: GroupingPlugin = _LazyPlugin() # type: ignore - course_copy: CourseCopyPlugin = _LazyPlugin() # type:ignore - misc: MiscPlugin = _LazyPlugin() # type:ignore + grouping = cast(GroupingPlugin, _LazyPlugin()) + course_copy = cast(CourseCopyPlugin, _LazyPlugin()) + misc = cast(MiscPlugin, _LazyPlugin()) def __init__(self, request, plugin_config: PluginConfig): self._request = request

robertknight · 2024-03-15T09:05:45Z

lms/services/http.py

@@ -7,7 +7,7 @@ class HTTPService:
    """Send HTTP requests with `requests` and receive the responses."""

    # This is here mostly to let auto-spec know about it in the tests
-    session: Session = None
+    session: Session = None  # typing: ignore


I had a look through the Python docs for create_autospec, which suggests some alternative ways of handling this. I include the diff below to illustrate, but # typing: ignore is simpler.

diff --git a/lms/services/http.py b/lms/services/http.py index 8d0e73fca..8c282b3c5 100644 --- a/lms/services/http.py +++ b/lms/services/http.py @@ -6,8 +6,7 @@ from lms.services.exceptions import ExternalRequestError class HTTPService: """Send HTTP requests with `requests` and receive the responses.""" - # This is here mostly to let auto-spec know about it in the tests - session: Session = None # typing: ignore + session: Session """The underlying requests Session.""" def __init__(self): @@ -15,7 +14,6 @@ class HTTPService: # requests and urllib3 connection pooling is used (which means that # underlying TCP connections are re-used when making multiple requests # to the same host, e.g. pagination). - # See https://docs.python-requests.org/en/latest/user/advanced/#session-objects self.session = Session() diff --git a/tests/unit/lms/services/jstor/service_test.py b/tests/unit/lms/services/jstor/service_test.py index d5152d0c3..ea4d86ad1 100644 --- a/tests/unit/lms/services/jstor/service_test.py +++ b/tests/unit/lms/services/jstor/service_test.py @@ -5,7 +5,7 @@ from unittest.mock import sentinel import pytest from lms.services import ExternalRequestError -from lms.services.jstor.service import ArticleNotFound, JSTORService +from lms.services.jstor.service import ArticleNotFound, HTTPService, JSTORService from tests import factories API_URL = "http://jstore_api.example.com" @@ -200,7 +200,12 @@ class TestJSTORService: @pytest.fixture(autouse=True) def HTTPService(self, patch): - return patch("lms.services.jstor.service.HTTPService") + class HTTPServiceForTest(HTTPService): + session = None + + return patch( + "lms.services.jstor.service.HTTPService", autospec=HTTPServiceForTest + ) @pytest.fixture(autouse=True) def JWTService(self, patch): diff --git a/tests/unit/lms/services/vitalsource/_client_test.py b/tests/unit/lms/services/vitalsource/_client_test.py index a0c43921e..9da609b36 100644 --- a/tests/unit/lms/services/vitalsource/_client_test.py +++ b/tests/unit/lms/services/vitalsource/_client_test.py @@ -7,6 +7,7 @@ from requests import Request from lms.services.exceptions import ExternalRequestError from lms.services.vitalsource._client import ( BookNotFound, + HTTPService, VitalSourceClient, VitalSourceConfigurationError, _VSUserAuth, @@ -253,9 +254,13 @@ class TestVitalSourceClient: @pytest.fixture(autouse=True) def http_service(self, patch): - HTTPService = patch("lms.services.vitalsource._client.HTTPService") + class HTTPServiceForTest(HTTPService): + session = None - return HTTPService.return_value + cls = patch( + "lms.services.vitalsource._client.HTTPService", autospec=HTTPServiceForTest + ) + return cls.return_value @pytest.fixture def _VSUserAuth(self, patch): diff --git a/tests/unit/services.py b/tests/unit/services.py index 6e71bc381..a43513842 100644 --- a/tests/unit/services.py +++ b/tests/unit/services.py @@ -1,6 +1,7 @@ from unittest import mock import pytest +from requests import Session from lms.product.plugin.course_copy import ( CourseCopyFilesHelper, @@ -234,9 +235,11 @@ def h_api(mock_service): @pytest.fixture def http_service(mock_service): - http_service = mock_service(HTTPService, service_name="http") - http_service.request.return_value = factories.requests.Response() + class HTTPServiceForTest(HTTPService): + session = mock.create_autospec(Session, spec_set=True, instance=True) + http_service = mock_service(HTTPServiceForTest, service_name="http") + http_service.request.return_value = factories.requests.Response() return http_service

acelaya · 2024-03-15T09:08:27Z

lms/services/http.py

@@ -7,7 +7,7 @@ class HTTPService:
    """Send HTTP requests with `requests` and receive the responses."""

    # This is here mostly to let auto-spec know about it in the tests
-    session: Session = None
+    session: Session = None  # typing: ignore


Should we then define the type as Session | None?

Add type annotations to columns that SQLAlchemy can't infer.

marcospri commented Mar 14, 2024

View reviewed changes

marcospri force-pushed the typing-fixes branch from f808505 to b443f36 Compare March 14, 2024 14:36

marcospri changed the title ~~Typing fixes~~ Miscellaneous python typing fixer Mar 14, 2024

marcospri commented Mar 14, 2024

View reviewed changes

marcospri force-pushed the typing-fixes branch 2 times, most recently from 09ee5fe to 48937ae Compare March 14, 2024 15:03

marcospri commented Mar 14, 2024

View reviewed changes

marcospri changed the title ~~Miscellaneous python typing fixer~~ Miscellaneous python typing fixes Mar 14, 2024

marcospri requested review from robertknight and acelaya March 14, 2024 15:20

robertknight approved these changes Mar 15, 2024

View reviewed changes

acelaya approved these changes Mar 15, 2024

View reviewed changes

marcospri force-pushed the typing-fixes branch from 48937ae to 220e015 Compare March 18, 2024 11:42

marcospri added 12 commits March 18, 2024 15:01

Typing fix: Pass explicitly as type

5b7654a

Typing fix: avoid redefining local variable's type

6c510e2

Typing fix: Fix optional declarations

cb6920f

Typing fix: Annotate model relationship

150f511

Typing fix: Ignore misc typing issues

214233a

Typing fix: Cast datetimes after parsing

c9ea77c

Typing fix: fix parameter default

c602a1c

Typing fix: Avoid re-typing local variable

44b6402

Enable mypy check after a round of fixes

26feb59

Typing fix: Narrow optional types with asserts

8d6c4c4

Typing fix: More direct access to the Family enum

60a33e5

Typing fix: use type casting instead of type ignore

d18fd65

marcospri force-pushed the typing-fixes branch from 220e015 to d18fd65 Compare March 18, 2024 14:02

marcospri added 3 commits March 18, 2024 16:24

Remove deprecated sqlalchemy typing plugin

2d0e9ff

Add type annotations to columns that SQLAlchemy can't infer.

Remove ignored error classes that don't occurs anymore in the codebase

b0daa88

Typing: Fix override mypy errors

2230f35

marcospri merged commit e913b61 into main Mar 19, 2024
8 checks passed

marcospri deleted the typing-fixes branch March 19, 2024 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous python typing fixes #6122

Miscellaneous python typing fixes #6122

marcospri commented Mar 14, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

acelaya Mar 14, 2024

robertknight Mar 15, 2024

marcospri Mar 18, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

marcospri Mar 14, 2024

robertknight Mar 15, 2024

acelaya Mar 15, 2024

marcospri Mar 14, 2024

robertknight left a comment

robertknight Mar 15, 2024

marcospri Mar 18, 2024

robertknight Mar 15, 2024

robertknight Mar 15, 2024

robertknight Mar 15, 2024

robertknight Mar 15, 2024

acelaya Mar 15, 2024

	grouping: GroupingPlugin = _LazyPlugin() # type: ignore
	grouping: GroupingPlugin = _LazyPlugin() # type:ignore

Miscellaneous python typing fixes #6122

Miscellaneous python typing fixes #6122

Conversation

marcospri commented Mar 14, 2024

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

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

robertknight 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

Choose a reason for hiding this comment