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

Misc typing fixes in not annotated functions #6389

Merged
merged 2 commits into from
Jun 26, 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/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

value_mapper: Callable | None = None

def __post_init__(self):
Expand Down
5 changes: 3 additions & 2 deletions lms/db/_columns.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import sqlalchemy as sa
from sqlalchemy.orm import Mapped, mapped_column


def varchar_enum( # noqa: PLR0913, PLR0917
Expand All @@ -8,9 +9,9 @@ def varchar_enum( # noqa: PLR0913, PLR0917
nullable=False,
server_default=None,
unique=False,
) -> sa.Column:
) -> Mapped:
"""Return a SA column type to store the python enum.Enum as a varchar in a table."""
return sa.Column(
return mapped_column(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

sa.Enum(
enum,
# In order to maintain maximum flexibility we will only enforce the
Expand Down
10 changes: 4 additions & 6 deletions lms/models/application_instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
requesters_email = sa.Column(sa.Unicode(2048), nullable=False)

last_launched = sa.Column(sa.DateTime(), nullable=True)
Expand Down Expand Up @@ -165,18 +165,16 @@ class ApplicationInstance(CreatedUpdatedMixin, Base):
files = sa.orm.relationship("File", back_populates="application_instance")

# LTIRegistration this instance belong to
lti_registration_id = sa.Column(
sa.Integer(),
sa.ForeignKey("lti_registration.id", ondelete="cascade"),
nullable=True,
lti_registration_id: Mapped[int | None] = mapped_column(
sa.ForeignKey("lti_registration.id", ondelete="cascade")
)

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(sa.UnicodeText)

role_overrides = sa.orm.relationship(
"LTIRoleOverride", back_populates="application_instance"
Expand Down
2 changes: 1 addition & 1 deletion lms/models/assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Assignment(CreatedUpdatedMixin, Base):
sa.UniqueConstraint("resource_link_id", "tool_consumer_instance_guid"),
)

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

resource_link_id = sa.Column(sa.Unicode, nullable=False)
"""The resource_link_id launch param of the assignment."""
Expand Down
6 changes: 2 additions & 4 deletions lms/models/group_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,10 @@ class GroupInfo(Base):
custom_canvas_course_id = sa.Column(sa.UnicodeText())

#: A dict of info about this group.
_info: Mapped[MutableDict | None] = mapped_column(
"info", MutableDict.as_mutable(JSONB())
)
_info: Mapped[dict | None] = mapped_column("info", MutableDict.as_mutable(JSONB()))

@property
def _safe_info(self):
def _safe_info(self) -> dict:
if self._info is None:
self._info = {}

Expand Down
2 changes: 1 addition & 1 deletion lms/models/grouping.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class Type(str, Enum):
nullable=False,
)

extra: Mapped[MutableDict] = mapped_column(
extra: Mapped[dict] = mapped_column(
MutableDict.as_mutable(JSONB()),
server_default=sa.text("'{}'::jsonb"),
nullable=False,
Expand Down
22 changes: 9 additions & 13 deletions lms/models/lti_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import sqlalchemy as sa
from sqlalchemy.ext.hybrid import hybrid_property
from sqlalchemy.orm import Mapped, mapped_column, relationship

from lms.db import Base, varchar_enum

Expand Down Expand Up @@ -39,27 +40,22 @@ class LTIRoleOverride(Base):

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

lti_role_id = sa.Column(
sa.Integer(),
sa.ForeignKey("lti_role.id", ondelete="cascade"),
nullable=True,
index=True,
lti_role_id: Mapped[int | None] = mapped_column(
sa.ForeignKey("lti_role.id", ondelete="cascade"), index=True
)
lti_role = sa.orm.relationship("LTIRole")
lti_role = relationship("LTIRole")

application_instance_id = sa.Column(
sa.Integer(),
sa.ForeignKey("application_instances.id", ondelete="cascade"),
nullable=False,
application_instance_id: Mapped[int] = mapped_column(
sa.ForeignKey("application_instances.id", ondelete="cascade")
)
application_instance = sa.orm.relationship(
application_instance = relationship(
"ApplicationInstance", back_populates="role_overrides"
)

type = varchar_enum(RoleType)
type: Mapped[RoleScope] = varchar_enum(RoleType)
"""Our interpretation of the value."""

scope = varchar_enum(RoleScope, nullable=True)
scope: Mapped[RoleType] = varchar_enum(RoleScope, nullable=True)
"""Scope where this role applies"""

@property
Expand Down
2 changes: 1 addition & 1 deletion lms/product/d2l/_plugin/course_copy.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
class D2LCourseCopyPlugin:
"""Handle course copy for D2L."""

file_type = "d2l_file"
file_type: str = "d2l_file"

def __init__(
self,
Expand Down
4 changes: 2 additions & 2 deletions lms/resources/_js_config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ def _config(self):

return config

def _get_product_info(self):
def _get_product_info(self) -> dict:
"""Return product (Canvas, BB, D2L..) configuration."""
product = self._request.product

Expand All @@ -591,7 +591,7 @@ def _get_product_info(self):
}

if self._request.product.settings.groups_enabled:
product_info["api"]["listGroupSets"] = {
product_info["api"]["listGroupSets"] = { # type: ignore
"authUrl": (
self._request.route_url(product.route.oauth2_authorize)
if product.route.oauth2_authorize
Expand Down
3 changes: 2 additions & 1 deletion lms/services/canvas.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
from lms.services.canvas_api.client import CanvasAPIClient
from lms.services.exceptions import CanvasAPIPermissionError, FileNotFoundInCourse


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

api = None
api: CanvasAPIClient = None # type:ignore
Copy link
Member Author

@marcospri marcospri Jun 25, 2024

Choose a reason for hiding this comment

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

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


def __init__(self, canvas_api, course_copy_plugin):
self.api = canvas_api
Expand Down
12 changes: 6 additions & 6 deletions lms/services/canvas_api/_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
from urllib.parse import urlencode

import requests
from requests import RequestException, Session
from requests import RequestException, Response, Session

from lms.services import CanvasAPIError, ExternalRequestError
from lms.services.exceptions import CanvasAPIError, ExternalRequestError


class BasicClient:
Expand Down Expand Up @@ -50,7 +50,7 @@ def send( # noqa: PLR0913, PLR0917
params=None,
headers=None,
url_stub="/api/v1",
):
) -> list:
"""
Make a request to the Canvas API and apply a schema to the response.

Expand Down Expand Up @@ -97,16 +97,16 @@ def _get_url(self, path, params, url_stub):
"?" + urlencode(params) if params else ""
)

def _send_prepared(self, request, schema, timeout, request_depth=1):
response = None
def _send_prepared(self, request, schema, timeout, request_depth=1) -> list:
response: Response = None # type:ignore

try:
response = self._session.send(request, timeout=timeout)
response.raise_for_status()
except RequestException as err:
CanvasAPIError.raise_from(err, request, response)

result = None
result: list = None # type: ignore
try:
result = schema(response).parse()
except ExternalRequestError as err:
Expand Down
4 changes: 2 additions & 2 deletions lms/services/canvas_api/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ class Meta:
course_id = fields.Integer(required=True)

@classmethod
def _ensure_sections_unique(cls, sections):
def _ensure_sections_unique(cls, sections) -> list:
"""
Ensure that sections returned by Canvas are unique.

Expand All @@ -528,7 +528,7 @@ def _ensure_sections_unique(cls, sections):
:return: A list of unique sections
:raise CanvasAPIError: When duplicate sections have different names
"""
sections_by_id = {}
sections_by_id: dict[int, dict] = {}

for section in sections:
duplicate = sections_by_id.get(section["id"])
Expand Down
2 changes: 1 addition & 1 deletion lms/services/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def any_with_setting(self, group, key, value=True) -> bool:
.count()
)

def get_from_launch(self, product, lti_params):
def get_from_launch(self, product, lti_params) -> Course:
"""Get the course this LTI launch based on the request's params."""
historical_course = None

Expand Down
2 changes: 1 addition & 1 deletion lms/services/d2l_api/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def group_set_groups(self, org_unit, group_category_id, user_id=None):

return groups

def list_files(self, org_unit) -> list[dict]:
def list_files(self, org_unit: str) -> list[dict]:
"""Get a nested list of files and folders for the given `org_unit`."""
modules = self._get_course_modules(org_unit)
files = list(self._find_files(org_unit, modules))
Expand Down
2 changes: 1 addition & 1 deletion lms/services/group_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def upsert_group_info(self, grouping: Grouping, params: dict):
# AI should be many:many, and we reflect that wrongness here.
group_info.application_instance = grouping.application_instance

group_info.type = self._GROUPING_TYPES[grouping.type] # type: ignore
group_info.type = self._GROUPING_TYPES[grouping.type]

for field in [
# Most (all) of these are duplicated elsewhere, we'll keep updating for now
Expand Down
2 changes: 1 addition & 1 deletion lms/services/organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
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 wrong

"""
Get an organization by its public_id.

Expand Down
4 changes: 1 addition & 3 deletions lms/views/admin/application_instance/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ def search_callback(self):
settings = None
if settings_key := self.request.params.get("settings_key"):
if settings_value := self.request.params.get("settings_value"):
settings_value = SETTINGS_BY_FIELD.get(settings_key).format(
settings_value
)
settings_value = SETTINGS_BY_FIELD[settings_key].format(settings_value)
else:
settings_value = ...

Expand Down
1 change: 1 addition & 0 deletions lms/views/admin/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def search(self):
if org_public_id := self.request.params.get("org_public_id", "").strip():
try:
organization = self.organization_service.get_by_public_id(org_public_id)
assert organization
organization_ids = self.organization_service.get_hierarchy_ids(
organization.id, include_parents=False
)
Expand Down
Loading