Skip to content
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

Enable mypy's check_untyped_defs #6444

Merged
merged 2 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lms/extensions/feature_flags/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ def feature(request, feature_flag_name):

def add_feature_flag_providers(_config, *providers):
"""Adapt feature_flags.add_providers()."""
providers = [config.maybe_dotted(provider) for provider in providers]
providers = [config.maybe_dotted(provider) for provider in providers] # type:ignore
return feature_flags.add_providers(*providers)

# Register the Pyramid request method and config directive. These are this
Expand Down
4 changes: 2 additions & 2 deletions lms/models/assignment.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import sqlalchemy as sa
from sqlalchemy.dialects.postgresql import JSONB
from sqlalchemy.ext.mutable import MutableDict
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import DynamicMapped, Mapped, mapped_column

from lms.db import Base
from lms.models._mixins import CreatedUpdatedMixin
Expand Down Expand Up @@ -78,7 +78,7 @@ class Assignment(CreatedUpdatedMixin, Base):
deep_linking_uuid: Mapped[str | None] = mapped_column(sa.Unicode, nullable=True)
"""UUID that identifies the deep linking that created this assignment."""

groupings: Mapped[list[Grouping]] = sa.orm.relationship(
groupings: DynamicMapped[Grouping] = sa.orm.relationship(
secondary="assignment_grouping", viewonly=True, lazy="dynamic"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note in the docs that lazy="dynamic" is a deprecated feature. Is it planned to migrate to the new thing at some point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigned this small note to myself, I don't think it's that urgent to migrate away.

We'd keep this in mind to avoid adding new usages of it.


Expand Down
10 changes: 4 additions & 6 deletions lms/models/oauth2_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from enum import Enum, unique

import sqlalchemy as sa
from sqlalchemy.orm import Mapped, mapped_column

from lms.db import Base, varchar_enum

Expand Down Expand Up @@ -43,7 +44,7 @@ class OAuth2Token(Base):
sa.UniqueConstraint("user_id", "application_instance_id", "service"),
)

id = sa.Column(sa.Integer(), autoincrement=True, primary_key=True)
id: Mapped[int] = mapped_column(autoincrement=True, primary_key=True)

#: The LTI user_id of the LMS user who this access token belongs to.
user_id = sa.Column(sa.UnicodeText(), nullable=False)
Expand Down Expand Up @@ -91,11 +92,8 @@ class OAuth2Token(Base):
#: inserting new rows) then whenever `access_token` and `expires_in`
#: are updated with new values from the authorization server, `received_at`
#: should also be updated to the current time.
received_at = sa.Column(
sa.DateTime,
default=datetime.datetime.utcnow,
server_default=sa.func.now(),
nullable=False,
received_at: Mapped[datetime.datetime] = mapped_column(
default=datetime.datetime.utcnow, server_default=sa.func.now()
)

#: The API that this token is used with. In OAuth 2.0 parlance, this
Expand Down
1 change: 1 addition & 0 deletions lms/product/plugin/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product/plugin code might need more time consuming refactor to typecheck.

Just ignoring it for now to be able to enable check_untyped_defs

from lms.product.plugin.course_copy import CourseCopyPlugin
from lms.product.plugin.grouping import GroupingPlugin
from lms.product.plugin.misc import MiscPlugin
Expand Down
4 changes: 2 additions & 2 deletions lms/product/plugin/course_copy.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Callable
from typing import Any, Callable

from lms.models import File
from lms.services.exceptions import ExternalRequestError, OAuth2TokenError
Expand All @@ -20,7 +20,7 @@ def is_file_in_course(self, course_id, file_id, type_) -> bool:

def find_matching_file_in_course(
self,
store_new_course_files: Callable[[str], None],
store_new_course_files: Callable[[str], Any] | Callable[[int], Any],
file_type: str,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful if this function had docs to clarify what the "non-obvious" parameters are and what the Any values here represent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any here is the return value, we don't use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... interesting. In TypeScript we use void as the return type for function arguments where the return value isn't used because any (arg: string) => T can be assigned to (arg: string) => void. This has an advantage over any because the type checker will complain if the function does try to use the argument. It seems that the same approach doesn't work here though if using Callable[[str], None].

This fails to typecheck:

from typing import Callable

def do_something(callback: Callable[[str], None]) -> None:
    callback("foo")

def bar(arg: str) -> str:
    return arg + arg

do_something(bar)

This does work, but requires boilerplate:

from typing import Callable, TypeVar

R = TypeVar("R", covariant=True)

def do_something(callback: Callable[[str], R]) -> None:
    callback("foo")

def bar(arg: str) -> str:
    return arg + arg

do_something(bar)

To be clear, this is just me being curious. I think Any is fine for this PR to get us started.

original_file_id,
new_course_id,
Expand Down
1 change: 1 addition & 0 deletions lms/product/plugin/plugin.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# type: ignore
from dataclasses import dataclass
from typing import cast

Expand Down
2 changes: 1 addition & 1 deletion lms/product/product.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from dataclasses import InitVar, dataclass

from lms.product.family import Family # pylint:disable=unused-import
from lms.product.plugin import PluginConfig, Plugins
from lms.product.plugin import PluginConfig, Plugins # type: ignore


@dataclass(frozen=True)
Expand Down
4 changes: 2 additions & 2 deletions lms/services/application_instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ def __init__(self, application_instance: ApplicationInstance):
)


def _email_or_domain_match(columns, email):
def _email_or_domain_match(columns, email: str):
"""
Get an SQL comparator for matching emails.

This will match the full email if it contains '@' or interpret the text
as a domain if not. This will search over all the provided fields.
"""
return sa.or_(
return sa.or_( # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the issue here? Also, can you add a comments alongside the type: ignore? In the frontend we have markers like this:

 // @ts-ignore - TS doesn't know about PDFViewerApplication global.

As TS fortunately allows us to add notes for future readers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other useful thing TS can do is to indicate that you expect an error:

 // @ts-expect-error - Navigation API is missing from TS

This will cause a warning or error if the suppression subsequently becomes unnecessary. Is there a mechanism in mypy to inform us if a # type: ignore becomes unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a mechanism in mypy to inform us if a # type: ignore becomes unnecessary?

Yes, we'll get an error for those with the current conf 👍

(
sa.func.lower(column) == email.lower()
if "@" in email
Expand Down
5 changes: 3 additions & 2 deletions lms/services/digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ def course_infos(self):

for row in self._db.execute(query):
# SQLAlchemy returns None instead of [].
authority_provided_ids = row.authority_provided_ids or []
row_authority_provided_ids = row.authority_provided_ids or []
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was re-typing a variable from the outer scope.

instructor_h_userids = row.instructor_h_userids or []

self._course_infos.append(
Expand All @@ -411,7 +411,8 @@ def course_infos(self):
learner_annotations=tuple(
annotation
for annotation in self.annotations
if annotation.authority_provided_id in authority_provided_ids
if annotation.authority_provided_id
in row_authority_provided_ids
and annotation.userid not in instructor_h_userids
),
)
Expand Down
4 changes: 2 additions & 2 deletions lms/services/moodle.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ def _construct_file_tree(course_id, files):
"children": [],
}
folders[component] = new_folder
current_node["children"].append(new_folder)
current_node["children"].append(new_folder) # type: ignore
current_node = folders[component]

file_node = {
Expand All @@ -266,7 +266,7 @@ def _construct_file_tree(course_id, files):
"lms_id": file_data["url"],
"updated_at": file_data["updated_at"],
}
current_node["children"].append(file_node)
current_node["children"].append(file_node) # type: ignore

return root["children"]

Expand Down
2 changes: 1 addition & 1 deletion lms/validation/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,5 @@ def wrapper_view(context, request):


def includeme(config):
_validated_view.options = ["schema"]
_validated_view.options = ["schema"] # type: ignore
config.add_view_deriver(_validated_view)
2 changes: 1 addition & 1 deletion lms/validation/_lti_launch_params.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def handle_error(self, error, data, *, many, **kwargs):
# ``err.messages``, but without overwriting any of the existing
# error messages already present in ``messages``.
for field in err.messages:
messages.setdefault(field, []).extend(err.messages[field])
messages.setdefault(field, []).extend(err.messages[field]) # type:ignore
return_url = None

if return_url:
Expand Down
2 changes: 1 addition & 1 deletion lms/views/admin/_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def deserialize(self, value, attr, data, **kwargs):
# pylint:disable=compare-to-empty-string
if value == missing or value.strip() == "":
return None
return super().deserialize(value, attr, data, **kwargs)
return super().deserialize(value, attr, data, **kwargs) # type:ignore


class EmptyStringInt(EmptyStringNoneMixin, fields.Int): # type: ignore
Expand Down
2 changes: 1 addition & 1 deletion lms/views/admin/assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def assignment_dashboard(self):
"dashboard.assignment",
public_id=assignment.groupings[
0
].application_instance.organization.public_id,
].application_instance.organization.public_id, # type: ignore
assignment_id=assignment.id,
),
)
Expand Down
3 changes: 2 additions & 1 deletion lms/views/admin/organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def __init__(self, request) -> None:
self.organization_service: OrganizationService = request.find_service(
OrganizationService
)
self.organization_usage_report_service: OrganizationService = (
self.organization_usage_report_service: OrganizationUsageReportService = (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the wrong annotation.

request.find_service(OrganizationUsageReportService)
)

Expand Down Expand Up @@ -155,6 +155,7 @@ def toggle_organization_enabled(self):
request_org_id = self.request.matchdict["id_"]
for org_id in self.organization_service.get_hierarchy_ids(request_org_id):
org = self.organization_service.get_by_id(org_id)
assert org, "Organization {org_id} not found"
self.organization_service.update_organization(
org, enabled=self.request.params.get("enabled", "") == "on"
)
Expand Down
6 changes: 3 additions & 3 deletions lms/views/api/blackboard/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ def via_url(self):
)

document_url = self.request.params["document_url"]
file_id = course.get_mapped_file_id(
DOCUMENT_URL_REGEX.search(document_url)["file_id"]
)
document_url_match = DOCUMENT_URL_REGEX.search(document_url)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few of these, assert that we expect the regex to always be successful.

assert document_url_match
file_id = course.get_mapped_file_id(document_url_match["file_id"])
try:
if self.request.lti_user.is_instructor:
if not self.course_copy_plugin.is_file_in_course(course_id, file_id):
Expand Down
2 changes: 1 addition & 1 deletion lms/views/api/canvas/authorize.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def oauth2_redirect_error(request):
)

if error_description := request.params.get("error_description"):
kwargs["error_details"] = {"error_description": error_description}
kwargs["error_details"] = {"error_description": error_description} # type: ignore

request.context.js_config.enable_oauth2_redirect_error_mode(**kwargs)

Expand Down
1 change: 1 addition & 0 deletions lms/views/api/canvas/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def via_url(self):
)

document_url_match = DOCUMENT_URL_REGEX.search(assignment.document_url)
assert document_url_match
public_url = self.canvas.public_url_for_file(
assignment,
document_url_match["file_id"],
Expand Down
1 change: 1 addition & 0 deletions lms/views/api/canvas/pages.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ def proxy(self):
@staticmethod
def _parse_document_url(document_url):
document_url_match = DOCUMENT_URL_REGEX.search(document_url)
assert document_url_match
course_id = document_url_match["course_id"]
page_id = document_url_match["page_id"]

Expand Down
2 changes: 1 addition & 1 deletion lms/views/api/d2l/authorize.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
permission=Permissions.API,
)
def authorize(request):
scopes = []
scopes: list[str] = []

if request.product.settings.files_enabled:
scopes += FILES_SCOPES
Expand Down
6 changes: 3 additions & 3 deletions lms/views/api/d2l/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ def via_url(_context, request):
course_id, raise_on_missing=True
)

file_id = course.get_mapped_file_id(
DOCUMENT_URL_REGEX.search(document_url)["file_id"]
)
document_url_match = DOCUMENT_URL_REGEX.search(document_url)
assert document_url_match
file_id = course.get_mapped_file_id(document_url_match["file_id"])
try:
if request.lti_user.is_instructor:
if not course_copy_plugin.is_file_in_course(course_id, file_id):
Expand Down
6 changes: 4 additions & 2 deletions lms/views/api/moodle/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ def via_url(_context, request):
)

document_url = request.params["document_url"]
document_course_id = DOCUMENT_URL_REGEX.search(document_url)["course_id"]
document_file_id = DOCUMENT_URL_REGEX.search(document_url)["url"]
document_url_match = DOCUMENT_URL_REGEX.search(document_url)
assert document_url_match
document_course_id = document_url_match["course_id"]
document_file_id = document_url_match["url"]
effective_file_id = _effective_file_id(
course_copy_plugin, course, document_url, document_course_id, document_file_id
)
Expand Down
1 change: 1 addition & 0 deletions lms/views/api/moodle/pages.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ def proxy(self):
@staticmethod
def _parse_document_url(document_url):
document_url_match = DOCUMENT_URL_REGEX.search(document_url)
assert document_url_match
course_id = document_url_match["course_id"]
page_id = document_url_match["page_id"]

Expand Down
2 changes: 1 addition & 1 deletion lms/views/lti/basic_launch.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ def _configure_assignment(self, assignment):
)

def _configure_js_for_file_picker(
self, assignment: Assignment, route: str = "configure_assignment"
self, assignment: Assignment | None, route: str = "configure_assignment"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_configure_js_for_file_picker takes an empty assignment when first creating an assignment.

) -> dict:
"""
Show the file-picker for the user to choose a document.
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ python_version = 3.11
warn_unused_configs = true
warn_redundant_casts = true
warn_unused_ignores = true

check_untyped_defs = true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


disable_error_code = [
"import-untyped",
Expand Down
Loading