From a131d6360863d3e168ab303ff0a9b878ce6e6439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Mon, 10 May 2021 11:34:15 +0200 Subject: [PATCH] feat: run mypy as part of testing the codebase The edx-platform codebase already includes quite a few type annotations, but they were not regularly checked. This may cause problems, when the annotations themselves include errors (as we found out in some of the learning_sequences annotations). So here, we add mypy as a dev requirement and introduce a make command to run mypy regularly. Mypy runs on a very small portion of the total edx-platform, as configured in mypy.ini. Our hope is that developers will add more and more modules to this configuration file, until we can eventually run mypy on the full code base. See discussion: https://discuss.openedx.org/t/dev-notes-running-mypy-on-edx-platform/4860 --- Makefile | 3 + mypy.ini | 6 ++ .../content/learning_sequences/admin.py | 29 +++++----- .../learning_sequences/api/outlines.py | 31 +++++----- .../learning_sequences/api/processors/base.py | 6 +- .../api/processors/content_gating.py | 7 ++- .../api/processors/schedule.py | 9 +-- .../content/learning_sequences/data.py | 7 +-- .../core/djangoapps/xblock/runtime/runtime.py | 2 +- openedx/core/types/__init__.py | 5 ++ openedx/core/types/admin.py | 57 +++++++++++++++++++ openedx/core/types/user.py | 7 +++ requirements/edx-sandbox/py35.txt | 1 - requirements/edx/development.in | 1 + requirements/edx/development.txt | 8 +++ 15 files changed, 131 insertions(+), 48 deletions(-) create mode 100644 mypy.ini create mode 100644 openedx/core/types/__init__.py create mode 100644 openedx/core/types/admin.py create mode 100644 openedx/core/types/user.py diff --git a/Makefile b/Makefile index 1c81d4afe6f5..093065efa44b 100644 --- a/Makefile +++ b/Makefile @@ -111,6 +111,9 @@ compile-requirements: ## Re-compile *.in requirements to *.txt upgrade: pre-requirements ## update the pip requirements files to use the latest releases satisfying our constraints $(MAKE) compile-requirements COMPILE_OPTS="--upgrade" +check-types: ## run static type-checking tests + mypy + # These make targets currently only build LMS images. docker_build: docker build . -f Dockerfile --target lms -t openedx/edx-platform diff --git a/mypy.ini b/mypy.ini new file mode 100644 index 000000000000..eee85b592ff7 --- /dev/null +++ b/mypy.ini @@ -0,0 +1,6 @@ +[mypy] +follow_imports = silent +ignore_missing_imports = True +allow_untyped_globals = True +exclude = tests +files = openedx/core/djangoapps/content/learning_sequences/,openedx/core/types diff --git a/openedx/core/djangoapps/content/learning_sequences/admin.py b/openedx/core/djangoapps/content/learning_sequences/admin.py index 6e032828d6f5..58ca0d3e713b 100644 --- a/openedx/core/djangoapps/content/learning_sequences/admin.py +++ b/openedx/core/djangoapps/content/learning_sequences/admin.py @@ -11,6 +11,7 @@ from opaque_keys import OpaqueKey import attr +from openedx.core import types from .api import get_content_errors, get_course_outline from .models import CourseContext, CourseSectionSequence @@ -82,33 +83,29 @@ def get_queryset(self, request): qs = qs.select_related('section', 'sequence') return qs + @types.admin_display(description="Title") def title(self, cs_seq): return cs_seq.sequence.title - title.short_description = "Title" + @types.admin_display(boolean=True) def accessible_after_due(self, cs_seq): return not cs_seq.inaccessible_after_due - accessible_after_due.boolean = True + @types.admin_display(boolean=True, description="Staff Only") def visible_to_staff_only(self, cs_seq): return not cs_seq.visible_to_staff_only - visible_to_staff_only.boolean = True - visible_to_staff_only.short_description = "Staff Only" + @types.admin_display(boolean=True, description="Timed Exam") def is_time_limited(self, cs_seq): return cs_seq.exam.is_time_limited - is_time_limited.boolean = True - is_time_limited.short_description = "Timed Exam" + @types.admin_display(boolean=True, description="Proctored Exam") def is_proctored_enabled(self, cs_seq): return cs_seq.exam.is_proctored_enabled - is_proctored_enabled.boolean = True - is_proctored_enabled.short_description = "Proctored Exam" + @types.admin_display(boolean=True, description="Practice Exam") def is_practice_exam(self, cs_seq): return cs_seq.exam.is_practice_exam - is_practice_exam.boolean = True - is_practice_exam.short_description = "Practice Exam" class CourseContextAdmin(admin.ModelAdmin): @@ -199,13 +196,13 @@ def get_queryset(self, request): qs = qs.prefetch_related('section_sequences') return qs + @types.admin_display(description="Record Created") def created(self, course_context): return course_context.learning_context.created - created.short_description = "Record Created" + @types.admin_display(description="Record Modified") def modified(self, course_context): return course_context.learning_context.modified - modified.short_description = "Record Modified" def course_key(self, course_context): return course_context.learning_context.context_key @@ -213,10 +210,10 @@ def course_key(self, course_context): def title(self, course_context): return course_context.learning_context.title + @types.admin_display(description="Published at (UTC)") def published_at(self, course_context): published_at_dt = course_context.learning_context.published_at return published_at_dt.strftime("%B %-d, %Y, %-I:%M %p") - published_at.short_description = "Published at (UTC)" def published_version(self, course_context): return course_context.learning_context.published_version @@ -227,17 +224,17 @@ def _publish_report_attr(self, course_context, attr_name): return None return getattr(learning_context.publish_report, attr_name) + @types.admin_display(description="Errors") def num_errors(self, course_context): return self._publish_report_attr(course_context, 'num_errors') - num_errors.short_description = "Errors" + @types.admin_display(description="Sections") def num_sections(self, course_context): return self._publish_report_attr(course_context, 'num_sections') - num_sections.short_description = "Sections" + @types.admin_display(description="Sequences") def num_sequences(self, course_context): return self._publish_report_attr(course_context, 'num_sequences') - num_sequences.short_description = "Sequences" def raw_outline(self, obj): """ diff --git a/openedx/core/djangoapps/content/learning_sequences/api/outlines.py b/openedx/core/djangoapps/content/learning_sequences/api/outlines.py index 60e4a00e9274..c08796e707e4 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/outlines.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/outlines.py @@ -6,15 +6,16 @@ import logging from collections import defaultdict from datetime import datetime -from typing import Dict, FrozenSet, Optional, List, Union -from django.contrib.auth import get_user_model -from django.db.models.query import QuerySet +from typing import Dict, FrozenSet, List, Optional, Union + from django.db import transaction +from django.db.models.query import QuerySet +from edx_django_utils.cache import TieredCache from edx_django_utils.monitoring import function_trace, set_custom_attribute from opaque_keys import OpaqueKey -from opaque_keys.edx.locator import LibraryLocator -from edx_django_utils.cache import TieredCache from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocator +from openedx.core import types from ..data import ( ContentErrorData, @@ -25,30 +26,28 @@ ExamData, UserCourseOutlineData, UserCourseOutlineDetailsData, - VisibilityData, - + VisibilityData ) from ..models import ( ContentError, + CourseContext, CourseSection, CourseSectionSequence, - CourseContext, CourseSequenceExam, LearningContext, LearningSequence, PublishReport, - UserPartitionGroup, + UserPartitionGroup ) from .permissions import can_see_all_content from .processors.content_gating import ContentGatingOutlineProcessor +from .processors.enrollment import EnrollmentOutlineProcessor +from .processors.enrollment_track_partition_groups import EnrollmentTrackPartitionGroupsOutlineProcessor from .processors.milestones import MilestonesOutlineProcessor from .processors.schedule import ScheduleOutlineProcessor from .processors.special_exams import SpecialExamsOutlineProcessor from .processors.visibility import VisibilityOutlineProcessor -from .processors.enrollment import EnrollmentOutlineProcessor -from .processors.enrollment_track_partition_groups import EnrollmentTrackPartitionGroupsOutlineProcessor -User = get_user_model() log = logging.getLogger(__name__) # Public API... @@ -255,7 +254,7 @@ def get_content_errors(course_key: CourseKey) -> List[ContentErrorData]: @function_trace('learning_sequences.api.get_user_course_outline') def get_user_course_outline(course_key: CourseKey, - user: User, + user: types.User, at_time: datetime) -> UserCourseOutlineData: """ Get an outline customized for a particular user at a particular time. @@ -272,7 +271,7 @@ def get_user_course_outline(course_key: CourseKey, @function_trace('learning_sequences.api.get_user_course_outline_details') def get_user_course_outline_details(course_key: CourseKey, - user: User, + user: types.User, at_time: datetime) -> UserCourseOutlineDetailsData: """ Get an outline with supplementary data like scheduling information. @@ -299,7 +298,7 @@ def get_user_course_outline_details(course_key: CourseKey, def _get_user_course_outline_and_processors(course_key: CourseKey, # lint-amnesty, pylint: disable=missing-function-docstring - user: User, + user: types.User, at_time: datetime): """ Helper function that runs the outline processors. @@ -352,7 +351,7 @@ def _get_user_course_outline_and_processors(course_key: CourseKey, # lint-amnes # Open question: Does it make sense to remove a Section if it has no Sequences in it? trimmed_course_outline = full_course_outline.remove(usage_keys_to_remove) - accessible_sequences = set(trimmed_course_outline.sequences) - inaccessible_sequences + accessible_sequences = frozenset(set(trimmed_course_outline.sequences) - inaccessible_sequences) user_course_outline = UserCourseOutlineData( base_outline=full_course_outline, diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/base.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/base.py index 773886144345..54182b282510 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/base.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/base.py @@ -5,10 +5,9 @@ import logging from datetime import datetime -from django.contrib.auth import get_user_model from opaque_keys.edx.keys import CourseKey # lint-amnesty, pylint: disable=unused-import +from openedx.core import types -User = get_user_model() log = logging.getLogger(__name__) @@ -33,7 +32,8 @@ class OutlineProcessor: additional methods to return specific metadata to feed into UserCourseOutlineDetailsData. """ - def __init__(self, course_key: CourseKey, user: User, at_time: datetime): + + def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime): """ Basic initialization. diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/content_gating.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/content_gating.py index 0d779161d5a3..a18a1fa5b7ff 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/content_gating.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/content_gating.py @@ -2,14 +2,14 @@ import logging from datetime import datetime -from django.contrib.auth import get_user_model from opaque_keys.edx.keys import CourseKey +from openedx.core import types + from common.djangoapps.student.models import EntranceExamConfiguration from common.djangoapps.util import milestones_helpers from .base import OutlineProcessor -User = get_user_model() log = logging.getLogger(__name__) @@ -21,7 +21,8 @@ class ContentGatingOutlineProcessor(OutlineProcessor): - Entrance Exams - Chapter gated content """ - def __init__(self, course_key: CourseKey, user: User, at_time: datetime): + + def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime): super().__init__(course_key, user, at_time) self.required_content = None self.can_skip_entrance_exam = False diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py index ec5eaeffeac7..024c17e3c2db 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py @@ -2,17 +2,18 @@ import logging from collections import defaultdict # lint-amnesty, pylint: disable=unused-import from datetime import datetime, timedelta +from typing import Dict -from django.contrib.auth import get_user_model from edx_when.api import get_dates_for_course from opaque_keys.edx.keys import CourseKey # lint-amnesty, pylint: disable=unused-import +from openedx.core import types + from common.djangoapps.student.auth import user_has_role from common.djangoapps.student.roles import CourseBetaTesterRole from ...data import ScheduleData, ScheduleItemData, UserCourseOutlineData from .base import OutlineProcessor -User = get_user_model() log = logging.getLogger(__name__) @@ -33,10 +34,10 @@ class ScheduleOutlineProcessor(OutlineProcessor): * Things that are made inaccessible after they're due. """ - def __init__(self, course_key: CourseKey, user: User, at_time: datetime): + def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime): super().__init__(course_key, user, at_time) self.dates = None - self.keys_to_schedule_fields = defaultdict(dict) + self.keys_to_schedule_fields: Dict[str, Dict[str, datetime]] = defaultdict(dict) self._course_start = None self._course_end = None self._is_beta_tester = False diff --git a/openedx/core/djangoapps/content/learning_sequences/data.py b/openedx/core/djangoapps/content/learning_sequences/data.py index 1ea8bd51b120..d7efe0e0a166 100644 --- a/openedx/core/djangoapps/content/learning_sequences/data.py +++ b/openedx/core/djangoapps/content/learning_sequences/data.py @@ -26,10 +26,9 @@ from typing import Dict, FrozenSet, List, Optional import attr -from django.contrib.auth import get_user_model from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx.core import types -User = get_user_model() log = logging.getLogger(__name__) @@ -211,7 +210,7 @@ def not_deprecated(self, _attribute, value): # derived from what you pass into `sections`. Do not set this directly. sequences = attr.ib(type=Dict[UsageKey, CourseLearningSequenceData], init=False) - course_visibility = attr.ib(validator=attr.validators.in_(CourseVisibility)) + course_visibility: CourseVisibility = attr.ib(validator=attr.validators.in_(CourseVisibility)) # Entrance Exam ID entrance_exam_id = attr.ib(type=str) @@ -339,7 +338,7 @@ class is a pretty dumb container that doesn't understand anything about how # Django User representing who we've customized this outline for. This may # be the AnonymousUser. - user: User + user: types.User # UTC TZ time representing the time for which this user course outline was # created. It is possible to create UserCourseOutlineData for a time in the diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 5f003e747925..d1e4b6a7a5b0 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -373,7 +373,7 @@ class can be used with many different XBlocks, whereas each XBlock gets its def __init__( self, - handler_url, # type: (Callable[[UsageKey, str, Union[int, ANONYMOUS_USER]], str] + handler_url, # type: Callable[[UsageKey, str, Union[int, ANONYMOUS_USER]], str] student_data_mode, # type: Union[STUDENT_DATA_EPHEMERAL, STUDENT_DATA_PERSISTED] runtime_class, # type: XBlockRuntime ): diff --git a/openedx/core/types/__init__.py b/openedx/core/types/__init__.py new file mode 100644 index 000000000000..15e03db2accd --- /dev/null +++ b/openedx/core/types/__init__.py @@ -0,0 +1,5 @@ +""" +Add here typing utilities API functions and classes. +""" +from .admin import admin_display +from .user import User diff --git a/openedx/core/types/admin.py b/openedx/core/types/admin.py new file mode 100644 index 000000000000..b6ff8c16d965 --- /dev/null +++ b/openedx/core/types/admin.py @@ -0,0 +1,57 @@ +""" +Typing utilities for the admin sites. +""" +import warnings + +from typing import Any, Callable, Optional, Protocol + + +class AdminMethod(Protocol): + """ + Duck-type definition of a callable admin method. + + See: + https://github.com/python/mypy/issues/2087#issuecomment-462726600 + https://mypy.readthedocs.io/en/stable/protocols.html + https://www.python.org/dev/peps/pep-0544/ + """ + + short_description: str + boolean: bool + + +def _admin_display( + boolean: Optional[bool] = None, description: Optional[str] = None +) -> Callable[[Any], AdminMethod]: + """ + Decorator for functions that need to be annotated with attributes from AdminMethod. + + This method and the above AdminMethod class will no longer be necessary in Django 3.2, + when `admin.display` is introduced: + https://docs.djangoproject.com/en/3.2/ref/contrib/admin/#django.contrib.admin.display + """ + + def decorator(func: Any) -> AdminMethod: + if boolean is not None: + func.boolean = boolean + if description is not None: + func.short_description = description + return func + + return decorator + + +try: + import django.contrib.admin + + admin_display = django.contrib.admin.display + if _admin_display or AdminMethod: + warnings.warn( + ( + "Django 3.2+ available: the _admin_display method and the AdminMethod" + "class should be removed from openedx.core.types" + ), + DeprecationWarning, + ) +except AttributeError: + admin_display = _admin_display diff --git a/openedx/core/types/user.py b/openedx/core/types/user.py new file mode 100644 index 000000000000..84f26a0aee3f --- /dev/null +++ b/openedx/core/types/user.py @@ -0,0 +1,7 @@ +""" +Typing utilities for the User models. +""" + +import django.contrib.auth.models + +User = django.contrib.auth.models.User diff --git a/requirements/edx-sandbox/py35.txt b/requirements/edx-sandbox/py35.txt index 336fc0540847..fde0371d3d78 100644 --- a/requirements/edx-sandbox/py35.txt +++ b/requirements/edx-sandbox/py35.txt @@ -62,7 +62,6 @@ numpy==1.16.5 # chem # matplotlib # openedx-calc - # scipy openedx-calc==1.0.9 # via -r requirements/edx-sandbox/py35.in pycparser==2.20 diff --git a/requirements/edx/development.in b/requirements/edx/development.in index 651478f38a56..2e32d08d8dcb 100644 --- a/requirements/edx/development.in +++ b/requirements/edx/development.in @@ -16,6 +16,7 @@ click # Used for perf_tests utilities in modulestore django-debug-toolbar # A set of panels that display debug information about the current request/response edx-sphinx-theme # Documentation theme +mypy # static type checking pywatchman # More efficient checking for runserver reload trigger events sphinxcontrib-openapi[markdown]==0.6.0 # OpenAPI (fka Swagger) spec renderer for Sphinx; pinned because 0.70 requires Python >=3.6 vulture # Detects possible dead/unused code, used in scripts/find-dead-code.sh diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 778a3d49fb19..9b424abe6d7e 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -853,6 +853,10 @@ mpmath==1.2.1 # via # -r requirements/edx/testing.txt # sympy +mypy-extensions==0.4.3 + # via mypy +mypy==0.812 + # via -r requirements/edx/development.in mysqlclient==2.0.3 # via -r requirements/edx/testing.txt newrelic==6.4.0.157 @@ -1377,6 +1381,10 @@ tqdm==4.61.0 # nltk transifex-client==0.14.2 # via -r requirements/edx/testing.txt +typed-ast==1.4.3 + # via mypy +typing-extensions==3.10.0.0 + # via mypy ua-parser==0.10.0 # via # -r requirements/edx/testing.txt