Enable mypy's check_untyped_defs #6444
Conversation
…s is enabled" This reverts commit ebbe824.
| @@ -1,3 +1,4 @@ | |||
| # type: ignore | |||
There was a problem hiding this comment.
Product/plugin code might need more time consuming refactor to typecheck.
Just ignoring it for now to be able to enable check_untyped_defs
Make necessary changes to allow a clean mypy run after the change
marcospri
force-pushed
the
mypy-functions-2
branch
from
899970e to
3253aba
Compare
| @@ -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 [] | |||
There was a problem hiding this comment.
This was re-typing a variable from the outer scope.
| @@ -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 = ( | |||
There was a problem hiding this comment.
This was the wrong annotation.
| file_id = course.get_mapped_file_id( | ||
| DOCUMENT_URL_REGEX.search(document_url)["file_id"] | ||
| ) | ||
| document_url_match = DOCUMENT_URL_REGEX.search(document_url) |
There was a problem hiding this comment.
There are a few of these, assert that we expect the regex to always be successful.
| @@ -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" | |||
There was a problem hiding this comment.
_configure_js_for_file_picker takes an empty assignment when first creating an assignment.
| @@ -225,7 +225,7 @@ python_version = 3.11 | |||
| warn_unused_configs = true | |||
| warn_redundant_casts = true | |||
| warn_unused_ignores = true | |||
|
|
|||
| check_untyped_defs = true | |||
There was a problem hiding this comment.
The main goal of this:
Type-checks the interior of functions without type annotations.
| @@ -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" | |||
| ) | |||
There was a problem hiding this comment.
I note in the docs that lazy="dynamic" is a deprecated feature. Is it planned to migrate to the new thing at some point?
There was a problem hiding this comment.
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.
| @@ -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], | |||
There was a problem hiding this comment.
It would be useful if this function had docs to clarify what the "non-obvious" parameters are and what the Any values here represent.
There was a problem hiding this comment.
Any here is the return value, we don't use it.
There was a problem hiding this comment.
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.
| """ | ||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
|
I'm going to merge this while Marcos is away to make sure that new code added subsequently is compatible with the |
Make necessary changes to allow a clean mypy run after the change