feat: enforce quality checks and type hints, improve quality and typi…

…ng (#73)

.github/workflows/ci.yml

@@ -16,7 +16,7 @@ jobs:
       matrix:
         os: [ubuntu-latest]  # Add macos-latest later?
         python-version: ['3.8']
-        toxenv: ["py38-django32", "py38-django42", "package"]
+        toxenv: ["py38-django32", "py38-django42", "package", "quality"]
         # We're only testing against MySQL 8 right now because 5.7 is
         # incompatible with Djagno 4.2. We'd have to make the tox.ini file more
         # complicated than it's worth given the short expected shelf-life of

.gitignore

@@ -26,6 +26,7 @@ pip-log.txt
 # Unit test / coverage reports
 .cache/
 .pytest_cache/
+.mypy_cache/
 .coverage
 .coverage.*
 .tox

docs/conf.py

@@ -524,6 +524,8 @@ def on_init(app):  # pylint: disable=unused-argument
 
 
 def setup(app):
-    """Sphinx extension: run sphinx-apidoc."""
+    """
+    Sphinx extension: run sphinx-apidoc.
+    """
     event = 'builder-inited'
     app.connect(event, on_init)

manage.py

@@ -18,7 +18,7 @@
         # issue is really that Django is missing to avoid masking other
         # exceptions on Python 2.
         try:
-            import django  # pylint: disable=unused-import, wrong-import-position
+            import django  # pylint: disable=unused-import
         except ImportError as import_error:
             raise ImportError(
                 "Couldn't import Django. Are you sure it's installed and "

mypy.ini

@@ -0,0 +1,15 @@
+[mypy]
+follow_imports = normal
+ignore_missing_imports = False
+allow_untyped_globals = False
+plugins =
+    mypy_django_plugin.main,
+    mypy_drf_plugin.main
+files =
+    openedx_learning,
+    openedx_tagging,
+    tests
+
+[mypy.plugins.django-stubs]
+# content_staging only works with CMS; others work with either, so we run mypy with CMS settings.
+django_settings_module = "projects.dev"

olx_importer/management/commands/load_components.py

@@ -46,7 +46,9 @@ def __init__(self, *args, **kwargs):
         self.init_known_types()
 
     def init_known_types(self):
-        """Intialize mimetypes with some custom mappings we want to use."""
+        """
+        Intialize mimetypes with some custom mappings we want to use.
+        """
         # This is our own hacky video transcripts related format.
         mimetypes.add_type("application/vnd.openedx.srt+json", ".sjson")
 

openedx_learning/__init__.py

@@ -1 +1,4 @@
+"""
+Open edX Learning ("Learning Core").
+"""
 __version__ = "0.1.5"

openedx_learning/contrib/media_server/__init__.py

@@ -0,0 +1,3 @@
+"""
+Media server (for dev or low-traffic instances)
+"""

openedx_learning/contrib/media_server/apps.py

@@ -1,6 +1,9 @@
+"""
+Django app metadata for the Media Server application.
+"""
 from django.apps import AppConfig
-from django.core.exceptions import ImproperlyConfigured
 from django.conf import settings
+from django.core.exceptions import ImproperlyConfigured
 
 
 class MediaServerConfig(AppConfig):

openedx_learning/contrib/media_server/urls.py

@@ -1,3 +1,6 @@
+"""
+URLs for the media server application
+"""
 from django.urls import path
 
 from .views import component_asset

openedx_learning/contrib/media_server/views.py

@@ -1,8 +1,12 @@
+"""
+Views for the media server application
+
+(serves media files in dev or low-traffic instances).
+"""
 from pathlib import Path
 
-from django.http import FileResponse
-from django.http import Http404
 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
+from django.http import FileResponse, Http404
 
 from openedx_learning.core.components.api import get_component_version_content
 
@@ -28,7 +32,7 @@ def component_asset(
             learning_package_key, component_key, version_num, asset_path
         )
     except ObjectDoesNotExist:
-        raise Http404("File not found")
+        raise Http404("File not found")  # pylint: disable=raise-missing-from
 
     if not cvc.learner_downloadable and not (
         request.user and request.user.is_superuser

openedx_learning/core/components/admin.py

@@ -1,34 +1,40 @@
+"""
+Django admin for components models
+"""
 from django.contrib import admin
 from django.template.defaultfilters import filesizeformat
 from django.urls import reverse
 from django.utils.html import format_html
+from django.utils.safestring import SafeText
 
-from .models import (
-    Component,
-    ComponentVersion,
-    ComponentVersionRawContent,
-)
 from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin
 
+from .models import Component, ComponentVersion, ComponentVersionRawContent
+
 
 class ComponentVersionInline(admin.TabularInline):
+    """
+    Inline admin view of ComponentVersion from the Component Admin
+    """
     model = ComponentVersion
     fields = ["version_num", "created", "title", "format_uuid"]
     readonly_fields = ["version_num", "created", "title", "uuid", "format_uuid"]
     extra = 0
 
+    @admin.display(description="UUID")
     def format_uuid(self, cv_obj):
         return format_html(
             '<a href="{}">{}</a>',
             reverse("admin:oel_components_componentversion_change", args=(cv_obj.pk,)),
             cv_obj.uuid,
         )
 
-    format_uuid.short_description = "UUID"
-
 
 @admin.register(Component)
 class ComponentAdmin(ReadOnlyModelAdmin):
+    """
+    Django admin configuration for Component
+    """
     list_display = ("key", "uuid", "namespace", "type", "created")
     readonly_fields = [
         "learning_package",
@@ -44,6 +50,9 @@ class ComponentAdmin(ReadOnlyModelAdmin):
 
 
 class RawContentInline(admin.TabularInline):
+    """
+    Django admin configuration for RawContent
+    """
     model = ComponentVersion.raw_contents.through
 
     def get_queryset(self, request):
@@ -75,11 +84,11 @@ def get_queryset(self, request):
     def rendered_data(self, cvc_obj):
         return content_preview(cvc_obj)
 
+    @admin.display(description="Size")
     def format_size(self, cvc_obj):
         return filesizeformat(cvc_obj.raw_content.size)
 
-    format_size.short_description = "Size"
-
+    @admin.display(description="Key")
     def format_key(self, cvc_obj):
         return format_html(
             '<a href="{}">{}</a>',
@@ -88,11 +97,12 @@ def format_key(self, cvc_obj):
             cvc_obj.key,
         )
 
-    format_key.short_description = "Key"
-
 
 @admin.register(ComponentVersion)
 class ComponentVersionAdmin(ReadOnlyModelAdmin):
+    """
+    Django admin configuration for ComponentVersion
+    """
     readonly_fields = [
         "component",
         "uuid",
@@ -120,29 +130,10 @@ def get_queryset(self, request):
         )
 
 
-def is_displayable_text(mime_type):
-    # Our usual text files, including things like text/markdown, text/html
-    media_type, media_subtype = mime_type.split("/")
-
-    if media_type == "text":
-        return True
-
-    # Our OLX goes here, but so do some other things like
-    if media_subtype.endswith("+xml"):
-        return True
-
-    # Other application/* types that we know we can display.
-    if media_subtype in ["json", "x-subrip"]:
-        return True
-
-    # Other formats that are really specific types of JSON
-    if media_subtype.endswith("+json"):
-        return True
-
-    return False
-
-
-def link_for_cvc(cvc_obj: ComponentVersionRawContent):
+def link_for_cvc(cvc_obj: ComponentVersionRawContent) -> str:
+    """
+    Get the download URL for the given ComponentVersionRawContent instance
+    """
     return "/media_server/component_asset/{}/{}/{}/{}".format(
         cvc_obj.raw_content.learning_package.key,
         cvc_obj.component_version.component.key,
@@ -151,14 +142,20 @@ def link_for_cvc(cvc_obj: ComponentVersionRawContent):
     )
 
 
-def format_text_for_admin_display(text):
+def format_text_for_admin_display(text: str) -> SafeText:
+    """
+    Get the HTML to display the given plain text (preserving formatting)
+    """
     return format_html(
         '<pre style="white-space: pre-wrap;">\n{}\n</pre>',
         text,
     )
 
 
-def content_preview(cvc_obj: ComponentVersionRawContent):
+def content_preview(cvc_obj: ComponentVersionRawContent) -> SafeText:
+    """
+    Get the HTML to display a preview of the given ComponentVersionRawContent
+    """
     raw_content_obj = cvc_obj.raw_content
 
     if raw_content_obj.mime_type.startswith("image/"):

openedx_learning/core/components/api.py

@@ -10,21 +10,29 @@
 Please look at the models.py file for more information about the kinds of data
 are stored in this app.
 """
+from __future__ import annotations
+
+from datetime import datetime
 from pathlib import Path
 
 from django.db.models import Q
 from django.db.transaction import atomic
 
-from ..publishing.api import (
-    create_publishable_entity,
-    create_publishable_entity_version,
-)
-from .models import ComponentVersionRawContent, Component, ComponentVersion
+from ..publishing.api import create_publishable_entity, create_publishable_entity_version
+from .models import Component, ComponentVersion, ComponentVersionRawContent
 
 
 def create_component(
-    learning_package_id, namespace, type, local_key, created, created_by
+    learning_package_id: int,
+    namespace: str,
+    type: str,  # pylint: disable=redefined-builtin
+    local_key: str,
+    created: datetime,
+    created_by: int | None,
 ):
+    """
+    Create a new Component (an entity like a Problem or Video)
+    """
     key = f"{namespace}:{type}@{local_key}"
     with atomic():
         publishable_entity = create_publishable_entity(
@@ -40,7 +48,16 @@ def create_component(
     return component
 
 
-def create_component_version(component_pk, version_num, title, created, created_by):
+def create_component_version(
+    component_pk: int,
+    version_num: int,
+    title: str,
+    created: datetime,
+    created_by: int | None,
+) -> ComponentVersion:
+    """
+    Create a new ComponentVersion
+    """
     with atomic():
         publishable_entity_version = create_publishable_entity_version(
             entity_id=component_pk,
@@ -57,8 +74,17 @@ def create_component_version(component_pk, version_num, title, created, created_
 
 
 def create_component_and_version(
-    learning_package_id, namespace, type, local_key, title, created, created_by
+    learning_package_id: int,
+    namespace: str,
+    type: str,  # pylint: disable=redefined-builtin
+    local_key: str,
+    title: str,
+    created: datetime,
+    created_by: int | None,
 ):
+    """
+    Create a Component and associated ComponentVersion atomically
+    """
     with atomic():
         component = create_component(
             learning_package_id, namespace, type, local_key, created, created_by
@@ -73,7 +99,7 @@ def create_component_and_version(
         return (component, component_version)
 
 
-def get_component_by_pk(component_pk):
+def get_component_by_pk(component_pk: int) -> Component:
     return Component.objects.get(pk=component_pk)
 
 
@@ -103,8 +129,14 @@ def get_component_version_content(
 
 
 def add_content_to_component_version(
-    component_version, raw_content_id, key, learner_downloadable=False
-):
+    component_version: ComponentVersion,
+    raw_content_id: int,
+    key: str,
+    learner_downloadable=False,
+) -> ComponentVersionRawContent:
+    """
+    Add a RawContent to the given ComponentVersion
+    """
     cvrc, _created = ComponentVersionRawContent.objects.get_or_create(
         component_version=component_version,
         raw_content_id=raw_content_id,

openedx_learning/core/components/apps.py

@@ -1,3 +1,6 @@
+"""
+Django metadata for the Components Django application.
+"""
 from django.apps import AppConfig
 
 
@@ -15,7 +18,7 @@ def ready(self):
         """
         Register Component and ComponentVersion.
         """
-        from ..publishing.api import register_content_models
-        from .models import Component, ComponentVersion
+        from ..publishing.api import register_content_models  # pylint: disable=import-outside-toplevel
+        from .models import Component, ComponentVersion  # pylint: disable=import-outside-toplevel
 
         register_content_models(Component, ComponentVersion)

openedx_learning/core/components/migrations/0001_initial.py

@@ -1,9 +1,11 @@
 # Generated by Django 3.2.19 on 2023-06-15 14:43
 
-from django.db import migrations, models
+import uuid
+
 import django.db.models.deletion
+from django.db import migrations, models
+
 import openedx_learning.lib.fields
-import uuid
 
 
 class Migration(migrations.Migration):