From 24882f7a4484a5f8f884437f8d65b45952089c25 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 30 Jan 2024 14:46:29 -0500 Subject: [PATCH] squash: more work on core, fields, plugin, mixins --- xblock/core.py | 46 ++++++++++++++++++++++++------------------ xblock/fields.py | 33 +++++++++++++++++------------- xblock/mixins.py | 7 +++++-- xblock/plugin.py | 52 +++++++++++++++++++++++++++++------------------- 4 files changed, 83 insertions(+), 55 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 41e1d410e..08972e59f 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -11,6 +11,7 @@ import warnings import typing as t from collections import defaultdict +from xml.etree import ElementTree as ET import pkg_resources from opaque_keys.edx.keys import LearningContextKey, UsageKey @@ -18,7 +19,8 @@ import xblock.exceptions from xblock.exceptions import DisallowedFileError -from xblock.fields import String, List, Scope +from xblock.fields import String, List, Scope, ScopeIds +from xblock.field_data import FieldData from xblock.internal import class_lazy import xblock.mixins from xblock.mixins import ( @@ -33,6 +35,10 @@ from xblock.plugin import Plugin from xblock.validation import Validation + +if t.TYPE_CHECKING: + from xblock.runtime import Runtime + # exposing XML_NAMESPACES as a member of core, in order to avoid importing mixins where # XML_NAMESPACES are needed (e.g. runtime.py). XML_NAMESPACES = xblock.mixins.XML_NAMESPACES @@ -87,7 +93,7 @@ def get_i18n_js_namespace(cls) -> str | None: return cls.i18n_js_namespace @classmethod - def open_local_resource(cls, uri: str) -> t.BinaryIO: + def open_local_resource(cls, uri: str) -> t.IO[bytes]: """ Open a local resource. @@ -143,11 +149,11 @@ class XBlock(XmlSerializationMixin, HierarchyMixin, ScopedStorageMixin, RuntimeS tags = List(help="Tags for this block", scope=Scope.settings) @class_lazy - def _class_tags(cls: type[XBlock]) -> set[str]: # pylint: disable=no-self-argument + def _class_tags(cls: type[XBlock]) -> set[str]: # type: ignore[misc] """ Collect the tags from all base classes. """ - class_tags = set() + class_tags: set[str] = set() for base in cls.mro()[1:]: # pylint: disable=no-member class_tags.update(getattr(base, '_class_tags', set())) @@ -165,7 +171,9 @@ def dec(cls: type[XBlock]) -> type[XBlock]: return dec @classmethod - def load_tagged_classes(cls, tag, fail_silently=True) -> t.Iterable[type[XBlock]]: + def load_tagged_classes( + cls, tag: str, fail_silently: bool = True + ) -> t.Iterable[tuple[str, type[XBlock]]]: """ Produce a sequence of all XBlock classes tagged with `tag`. @@ -178,18 +186,17 @@ def load_tagged_classes(cls, tag, fail_silently=True) -> t.Iterable[type[XBlock] (e.g. on startup or first page load), and in what contexts. Hence, the flag. """ - # Allow this method to access the `_class_tags` - # pylint: disable=W0212 for name, class_ in cls.load_classes(fail_silently): - if tag in class_._class_tags: - yield name, class_ + xblock_class: type[XBlock] = class_ # type: ignore + if tag in xblock_class._class_tags: # pylint: disable=protected-access + yield name, xblock_class # pylint: disable=keyword-arg-before-vararg def __init__( self, runtime: Runtime, field_data: FieldData | None = None, - scope_ids: ScopeIds = UNSET, + scope_ids: ScopeIds | object = UNSET, *args, **kwargs ): @@ -264,7 +271,7 @@ def ugettext(self, text) -> str: runtime_ugettext = runtime_service.ugettext return runtime_ugettext(text) - def add_xml_to_node(self, node: etree.Element) -> None: + def add_xml_to_node(self, node: ET.Element) -> None: """ For exporting, set data on etree.Element `node`. """ @@ -273,18 +280,19 @@ def add_xml_to_node(self, node: etree.Element) -> None: self.add_children_to_node(node) -XBlockAsideView: t.TypeAlias = t.Callable[[XBlockAside, XBlock, dict | None], Fragment] +# An XBlockAside's view method takes itself, an XBlock, and optional context dict, +# and returns a Fragment. +AsideView = t.Callable[["XBlockAside", XBlock, t.Optional[dict]], Fragment] class XBlockAside(XmlSerializationMixin, ScopedStorageMixin, RuntimeServicesMixin, HandlersMixin, SharedBlockBase): """ This mixin allows Xblock-like class to declare that it provides aside functionality. """ - entry_point: str = "xblock_asides.v1" @classmethod - def aside_for(cls, view_name: str) -> t.Callable[[XBlockAsideView], XBlockAsideView]: + def aside_for(cls, view_name: str) -> t.Callable[[AsideView], AsideView]: """ A decorator to indicate a function is the aside view for the given view_name. @@ -297,11 +305,11 @@ def student_aside(self, block, context=None): """ # pylint: disable=protected-access - def _decorator(func: XBlockAsideView) -> XBlockAsideView: + def _decorator(func: AsideView) -> AsideView: if not hasattr(func, '_aside_for'): - func._aside_for = [] + func._aside_for = [] # type: ignore - func._aside_for.append(view_name) # pylint: disable=protected-access + func._aside_for.append(view_name) # type: ignore return func return _decorator @@ -321,14 +329,14 @@ def _combined_asides(cls) -> dict[str, str | None]: # pylint: disable=no-self-a """ # The method declares what views it decorates. We rely on `dir` # to handle subclasses and overrides. - combined_asides = defaultdict(None) + combined_asides: dict[str, str | None] = defaultdict(None) for _view_name, view_func in inspect.getmembers(cls, lambda attr: hasattr(attr, '_aside_for')): aside_for = getattr(view_func, '_aside_for', []) for view in aside_for: combined_asides[view] = view_func.__name__ return combined_asides - def aside_view_declaration(self, view_name: str) -> XBlockAsideView | None: + def aside_view_declaration(self, view_name: str) -> AsideView | None: """ Find and return a function object if one is an aside_view for the given view_name diff --git a/xblock/fields.py b/xblock/fields.py index 57caa62db..932b0ce6d 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -57,14 +57,7 @@ class Sentinel: """ Class for implementing sentinel objects (only equal to themselves). """ - name: str = "" # Name is 'optional' for ease of making sub-class instances. - - def __post_init__(self): - """ - Ensure `self.name` is set, either by constructor or subclass. - """ - if not self.name: - raise ValueError("Coding error: sentinel must have a name!") + name: str def __repr__(self) -> str: return self.name @@ -120,6 +113,11 @@ def scopes(cls) -> list[BlockScope]: """ return list(cls) + @property + def attr_name(self) -> str: + return self.name.lower().replace('.', '_') + + class UserScope(Enum): """ @@ -154,6 +152,10 @@ def scopes(cls) -> list[UserScope]: """ return list(cls) + @property + def attr_name(self) -> str: + return self.name.lower().replace('.', '_') + class ScopeBase(t.NamedTuple): user: UserScope @@ -224,9 +226,12 @@ def scopes(cls) -> list[ScopeBase]: if cls(user, block) not in named_scopes ] - def __new__(cls, user: UserScope, block, name: str | None = None) -> Scope: + def __new__(cls, user: UserScope, block, name: str | None = None) -> Scope: # type: ignore """Create a new Scope, with an optional name.""" - return cls(user, block, name or f'{user}_{block}') + # TODO: This is a pretty wacky way to set a default value for `name`. + # We should try to refactor this so that Scope is just, like, + # a dataclass with a __post_init__ hook that sets the default `name`. + return ScopeBase.__new__(cls, user, block, name or f'{user}_{block}') children: t.ClassVar = Sentinel('Scope.children') parent: t.ClassVar = Sentinel('Scope.parent') @@ -262,7 +267,7 @@ class Unset(Sentinel): """ Indicates that default value has not been provided. """ - name = "fields.UNSET" + name: str = "fields.UNSET" @dataclass(frozen=True) @@ -272,7 +277,7 @@ class UniqueId(Sentinel): definition to signal that the field should default to a unique string value calculated at runtime. """ - name = "fields.UNIQUE_ID" + name: str = "fields.UNIQUE_ID" @dataclass(frozen=True) @@ -281,7 +286,7 @@ class NoCacheValue(Sentinel): Placeholder ('nil') value to indicate when nothing has been stored in the cache ("None" may be a valid value in the cache, so we cannot use it). """ - name = "fields.NO_CACHE_VALUE" + name: str = "fields.NO_CACHE_VALUE" @dataclass(frozen=True) @@ -290,7 +295,7 @@ class ExplicitlySet(Sentinel): Placeholder value that indicates that a value is explicitly dirty, because it was explicitly set. """ - name = "fields.EXPLICITLY_SET" + name: str = "fields.EXPLICITLY_SET" # For backwards API compatibility, define an instance of each Field-related sentinel. diff --git a/xblock/mixins.py b/xblock/mixins.py index e921caf80..962cf53e2 100644 --- a/xblock/mixins.py +++ b/xblock/mixins.py @@ -2,6 +2,8 @@ This module defines all of the Mixins that provide components of XBlock-family functionality, such as ScopeStorage, RuntimeServices, and Handlers. """ +from __future__ import annotations + from collections import OrderedDict import copy import functools @@ -14,7 +16,8 @@ from webob import Response from xblock.exceptions import JsonHandlerError, KeyValueMultiSaveError, XBlockSaveError, FieldDataDeprecationWarning -from xblock.fields import Field, Reference, Scope, ReferenceList +from xblock.fields import Field, Reference, Scope, ScopeIds, ReferenceList +from xblock.field_data import FieldData from xblock.internal import class_lazy, NamedAttributesMetaclass @@ -188,7 +191,7 @@ def fields(cls): # pylint: disable=no-self-argument return fields - def __init__(self, scope_ids, field_data=None, **kwargs): + def __init__(self, scope_ids: ScopeIds, field_data: FieldData | None = None, **kwargs): """ Arguments: field_data (:class:`.FieldData`): Interface used by the XBlock diff --git a/xblock/plugin.py b/xblock/plugin.py index 42f1ca6ce..e72f2d253 100644 --- a/xblock/plugin.py +++ b/xblock/plugin.py @@ -3,16 +3,20 @@ This code is in the Runtime layer. """ +from __future__ import annotations + import functools import itertools import logging -import pkg_resources +import typing as t + +from pkg_resources import iter_entry_points, EntryPoint from xblock.internal import class_lazy log = logging.getLogger(__name__) -PLUGIN_CACHE = {} +PLUGIN_CACHE: dict[tuple[str, str], type[Plugin]] = {} class PluginMissingError(Exception): @@ -21,35 +25,38 @@ class PluginMissingError(Exception): class AmbiguousPluginError(Exception): """Raised when a class name produces more than one entry_point.""" - def __init__(self, all_entry_points): + def __init__(self, all_entry_points: list[EntryPoint]): classes = (entpt.load() for entpt in all_entry_points) desc = ", ".join("{0.__module__}.{0.__name__}".format(cls) for cls in classes) msg = f"Ambiguous entry points for {all_entry_points[0].name}: {desc}" super().__init__(msg) -def default_select(identifier, all_entry_points): # pylint: disable=inconsistent-return-statements +def default_select(identifier: str, all_entry_points: list[EntryPoint]) -> EntryPoint: """ - Raise an exception when we have ambiguous entry points. + Raise an exception when we have no entry points or ambiguous entry points. """ - - if len(all_entry_points) == 0: + if not all_entry_points: raise PluginMissingError(identifier) - if len(all_entry_points) == 1: - return all_entry_points[0] elif len(all_entry_points) > 1: raise AmbiguousPluginError(all_entry_points) + return all_entry_points[0] class Plugin: - """Base class for a system that uses entry_points to load plugins. + """ + Base class for a system that uses entry_points to load plugins. Implementing classes are expected to have the following attributes: `entry_point`: The name of the entry point to load plugins from. - """ - entry_point = None # Should be overwritten by children classes + # TODO: In Python 3.11+, replace all annotations of `type[Plugin]` with + # `type[t.Self]`. The former is correct, but the latter is even better, + # because it'd mean that `XBlock.load_class` returns `type[XBlock]` instead + # of `type[Plugin]`. + + entry_point: str # Should be overwritten by children classes @class_lazy def extra_entry_points(cls): # pylint: disable=no-self-argument @@ -62,7 +69,7 @@ def extra_entry_points(cls): # pylint: disable=no-self-argument return [] @classmethod - def _load_class_entry_point(cls, entry_point): + def _load_class_entry_point(cls, entry_point: EntryPoint) -> type[Plugin]: """ Load `entry_point`, and set the `entry_point.name` as the attribute `plugin_name` on the loaded object @@ -72,7 +79,12 @@ def _load_class_entry_point(cls, entry_point): return class_ @classmethod - def load_class(cls, identifier, default=None, select=None): + def load_class( + cls, + identifier: str, + default: type[Plugin] | None = None, + select: t.Callable[[str, list[EntryPoint]], EntryPoint] | None = None, + ) -> type[Plugin]: """Load a single class specified by identifier. If `identifier` specifies more than a single class, and `select` is not None, @@ -100,7 +112,7 @@ def select(identifier, all_entry_points): if select is None: select = default_select - all_entry_points = list(pkg_resources.iter_entry_points(cls.entry_point, name=identifier)) + all_entry_points = list(iter_entry_points(cls.entry_point, name=identifier)) for extra_identifier, extra_entry_point in iter(cls.extra_entry_points): if identifier == extra_identifier: all_entry_points.append(extra_entry_point) @@ -117,7 +129,7 @@ def select(identifier, all_entry_points): return PLUGIN_CACHE[key] @classmethod - def load_classes(cls, fail_silently=True): + def load_classes(cls, fail_silently: bool = True) -> t.Iterable[tuple[str, type[Plugin]]]: """Load all the classes for a plugin. Produces a sequence containing the identifiers and their corresponding @@ -133,7 +145,7 @@ def load_classes(cls, fail_silently=True): contexts. Hence, the flag. """ all_classes = itertools.chain( - pkg_resources.iter_entry_points(cls.entry_point), + iter_entry_points(cls.entry_point), (entry_point for identifier, entry_point in iter(cls.extra_entry_points)), ) for class_ in all_classes: @@ -146,15 +158,15 @@ def load_classes(cls, fail_silently=True): raise @classmethod - def register_temp_plugin(cls, class_, identifier=None, dist='xblock'): - """Decorate a function to run with a temporary plugin available. + def register_temp_plugin(cls, class_: type, identifier: str | None = None, dist: str = 'xblock'): + """ + Decorate a function to run with a temporary plugin available. Use it like this in tests:: @register_temp_plugin(MyXBlockClass): def test_the_thing(): # Here I can load MyXBlockClass by name. - """ from unittest.mock import Mock # pylint: disable=import-outside-toplevel