feat!: collapse extraneous XBlock mixins #718

kdmccormick · 2024-02-12T18:14:03Z

Supporting info

Proposed in this DEPR issue:

[DEPR]: Extraneous XBlock Mixins #714

excerpt:

The implementations of the XBlock and XBlockAside classes are chopped into about a dozen tiny mixins. This was done many years ago, probably for some mix of reasons including code organization and theoretical reusability. Now, in 2024 in the openedx GitHub org, there are uses of the XBlock, XBlockAside, and XBlockMixin classes, but we do not see uses of any of the other classes or mixins (other than one extraneous reference to HandlersMixin in edx-platform, which can be removed).

Unfortunately, these mixins make it harder/impossible to write robust type constraints for XBlock and XBlockAside. They also make it harder to find code in the XBlock package. See discussion.

The edx-platform upgrade PR is here:

chore: upgrade to XBlock==3.1.0 edx-platform#34231

Description

Various extraneous classes have been removed from the XBlock API. We believe that most, if not all, XBlock API users will be unaffected by this change.

Diagram

Generated with pyreverse and dot.

Before

After

Change list

Removed:
- xblock.XBlockMixin (still available as xblock.core.XBlockMixin)
- xblock.core.SharedBlockBase (replaced with xblock.core.Blocklike)
- xblock.internal.Nameable
- xblock.internal.NamedAttributesMetaclass
- xblock.django.request.HeadersDict
- xblock.fields.XBlockMixin (still available as xblock.core.XBlockMixin)
- xblock.mixins.RuntimeServicesMixin
- xblock.mixins.ScopedStorageMixin
- xblock.mixins.IndexInfoMixin
- xblock.mixins.XmlSerializationMixin
- xblock.mixins.HandlersMixin
- xblock.mixins.ChildrenModelMetaclass
- xblock.mixins.HierarchyMixin
- xblock.mixins.ViewsMixin
Added:
- xblock.core.Blocklike, the new common ancestor of XBlock and XBlockAside, and XBlockMixin, replacing xblock.core.SharedBlockBase.
- New attributes on xblock.core.XBlockAside, each behaving the same as their XBlock counterpart:
  - usage_key
  - context_key
  - index_dictionary
- Various new attributes on xblock.core.XBlockMixin, encompassing the functionality of these former classes:
  - xblock.mixins.IndexInfoMixin
  - xblock.mixins.XmlSerializationMixin
  - xblock.mixins.HandlersMixin
Docs
- Various docstrings have been improved, some of which are published in the docs.
- XBlockAside will now be represented in the API docs, right below XBlock on the "XBlock API" page.
- XBlockMixin has been removed from the docs. It was only ever documented under the "Fields API" page (which didn't make any sense), and it was barely even documented there. We considered adding it back to the "XBlock API" page, but as noted in the class's new docstring, we do not want to encourage any new use of XBlockMixin.

Bumps version from 2.0.0 to 3.0.0.

Testing

Docs

You can preview the doc additions by clicking "Details" on the docs build, hitting "View Docs", and going to the "XBlock API" page).

Homepage: https://docsopenedxorg--718.org.readthedocs.build/projects/xblock/en/718/index.html
New XBlockAside generated docs: https://docsopenedxorg--718.org.readthedocs.build/projects/xblock/en/718/xblock.html#xblock.core.XBlockAside

XBlock and XBlockAside attributes

From the root of the repo, you can run this script in order to compare the list of attributes on XBlock/XBlockAside before and after this PR, both for the classes and their instances.

rm -rf dir_before dir_after
mkdir dir_before dir_after

cd dir_before
git switch master
python -c 'from xblock import core; print(*sorted(dir(core.XBlock)), sep="\n")' > block_cls.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlock(None, None, None))), sep="\n")' > block_obj.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlockAside)), sep="\n")'  > aside_cls.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlockAside(None, None, runtime=None))), sep="\n")' > aside_obj.list
cd ..

cd dir_after
git switch kdmccormick/mixins
python -c 'from xblock import core; print(*sorted(dir(core.XBlock)), sep="\n")' > block_cls.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlock(None, None, None))), sep="\n")' > block_obj.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlockAside)), sep="\n")'  > aside_cls.list
python -c 'from xblock import core; print(*sorted(dir(core.XBlockAside(None, None, runtime=None))), sep="\n")' > aside_obj.list
cd ..

diff dir_before list dir_after

This should yield:

diff dir_before/aside_cls.list dir_after/aside_cls.list
39a40
> context_key
49a51
> index_dictionary
62a65
> usage_key
diff dir_before/aside_obj.list dir_after/aside_obj.list
42a43
> context_key
52a54
> index_dictionary
67a70
> usage_key
diff dir_before/block_cls.list dir_after/block_cls.list
0a1
> __annotations__
diff dir_before/block_obj.list dir_after/block_obj.list
0a1
> __annotations__

which indicates that:

XBlockAsides now have context_key, usage_key and index_dictionary attributes
XBlocks have a few type annotations now (more to come in Add type hints and check them in CI #707 )
The list of attributes is otherwise unchanged

kdmccormick

Notes for reviewers 👀

kdmccormick · 2024-02-13T16:13:38Z

docs/conf.py

@@ -128,6 +128,7 @@
    ('py:class', 'aside'),
    ('py:class', 'aside_fn'),
    ('py:class', 'webob.Request'),
+    ('py:class', 'webob.Response'),


Added so that we can reference the "Response" class in a new docstring.

kdmccormick · 2024-02-13T16:14:12Z

xblock/__init__.py

-class XBlockMixin(xblock.core.XBlockMixin):
-    """
-    A wrapper around xblock.core.XBlockMixin that provides backwards compatibility for the old location.
-
-    Deprecated.
-    """
-    def __init__(self, *args, **kwargs):
-        warnings.warn("Please use xblock.core.XBlockMixin", DeprecationWarning, stacklevel=2)
-        super().__init__(*args, **kwargs)
-
-
-# For backwards compatibility, provide the XBlockMixin in xblock.fields
-# without causing a circular import
-xblock.fields.XBlockMixin = XBlockMixin


We're just removing the wrapper locations, which have been deprecated since 2014. XBlockMixin is still available at xblock.core.XBlockMixin.

kdmccormick · 2024-02-13T16:14:47Z

xblock/core.py

+# OrderedDict is used so that namespace attributes are put in predictable order
+# This allows for simple string equality assertions in tests and have no other effects
+XML_NAMESPACES = OrderedDict([
+    ("option", "http://code.edx.org/xblock/option"),
+    ("block", "http://code.edx.org/xblock/block"),
+])


Comment copied in from xblock.mixins

kdmccormick · 2024-02-13T16:14:56Z

xblock/core.py


 # __all__ controls what classes end up in the docs.
-__all__ = ['XBlock']
+__all__ = ['XBlock', 'XBlockAside']


This puts XBlockAside in the docs

kdmccormick · 2024-02-13T16:18:50Z

xblock/core.py

 UNSET = object()


-class XBlockMixin(ScopedStorageMixin):
+class _AutoNamedFieldsMetaclass(type):


xblock.internal.NamedAttributesMetaclass was responsible for sticking __name__ attributes on any Nameable objects defined on its classes. In practice, Field was the only type of Nameable attribute.

_AutoNamedFieldsMetaclass is essentially the same as NamedAttributesMetaclass, but to simplify things, it only just operates on Fields.

_AutoNamedFieldsMetaclass continues to be the metaclass for both XBlock and XBlockAside (although XBlock further specializes it as _HasChildrenMetaclass, defined further down).

kdmccormick · 2024-02-13T16:29:42Z

xblock/core.py

-        super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, *args, **kwargs)
-
-    @property
-    def usage_key(self):


usage_key and context_key are moved to Blocklike so that XBlockAsides can use them too.

kdmccormick · 2024-02-13T16:32:20Z

xblock/fields.py

+    @staticmethod
+    def needs_name(field):
+        """
+        Returns whether the given ) is yet to be named.
+        """
+        return not field.__name__
+


This was copied in from Nameable, but I recall now that I've refactored _AutoNameFieldsMetaclass such that this is now unused. I'll remove it before merging.

Suggested change

@staticmethod

def needs_name(field):

"""

Returns whether the given ) is yet to be named.

"""

return not field.__name__

kdmccormick · 2024-02-13T16:33:41Z

xblock/internal.py

@@ -28,42 +27,3 @@ def __get__(self, instance, owner):


 class_lazy = LazyClassProperty  # pylint: disable=invalid-name
-
-
-class NamedAttributesMetaclass(type):


NamedAttributesMetaclass and Nameable have been de-generalized into xblock.core._AutoNamedFieldsMetaclass and xblock.fields.Field, respectively.

kdmccormick · 2024-02-13T16:37:28Z

xblock/test/test_core.py

+    @Blocklike.needs("field-data")
+    class FieldTester(Blocklike):


RuntimeServicesMixin defined the @needs decorator, which allowed its subclass ScopedStorageMixin to invoke @needs("field-data").

However, now Blocklike defines @needs, so Blocklike itself cannot invoke @needs("field-data").

Thus, XBlock and XBlockAside now each invoke @needs("field-data") individually, and thus tests that subclass Blocklike must also invoke @needs("field-data").

Can you manually invoke the decorator on Blocklike after it's defined? I haven't done something like this in ages, but I think it'd be something like this?

class Blocklike: # lots of stuff here Blocklike = Blocklike.needs(Blocklike, "field-data")

kdmccormick · 2024-02-13T16:42:00Z

xblock/test/test_parsing.py

-            patched_warn.assert_called_once_with("XBlock %s does not contain field %s", type(block), parameter_name)
+            patched_warn.assert_called_once_with("%s does not contain field %s", type(block), parameter_name)


This warning was incorrect when the object was an XBlockAside.

I considered changing it to "Blocklike", but then the message would be:

Blocklike FooBlock does not contain field bar

which isn't any better than:

FooBlock does not contain field bar.

I could be convinced otherwise, esp if folks feel that the former is more regex-able.

Various extraneous classes have been removed from the XBlock API, simplifying its implementation and making debugging of XBlock instances easier. We believe that most, if not all, XBlock API users will be unaffected by this change. Some improvements have also been made to the reference documentation. See CHANGELOG.rst for details. Closes: openedx#714

ormsbee

Just some comments/questions. No major concerns.

ormsbee · 2024-03-19T03:52:27Z

xblock/core.py

+    (...or don't do that, because it's confusing for other developers, and linters/type-checkers
+        won't understand that the mixins are part of XBlock instances, so your code will be full
+        of ignore statements :)


I appreciate the sentiment here, but this statement makes it sound like there's a different, recommended course of action, which should be elaborated on if it's the case.

@ormsbee You know, on second thought, what I really want is for people to avoid adding new xblock mixins to the edx-platform runtime. If people want to add new xblock mixins to their custom fork or custom runtime, that's A-OK. I'll move my big DO-NOT-USE-THIS comment to edx-platform instead.

openedx/edx-platform@aa8672b#diff-a16fd1c6cf75bdebdf97a2633b533d39e43975c6e98af58787ed6af1282eca71R980-R984

ormsbee · 2024-03-19T04:04:25Z

xblock/core.py

+            node.set(field_name, text_value)
+
+
+class XBlockMixin(Blocklike):


If XBlockMixin is a subclass of Blocklike that overrides no other behavior, why not rename Blocklike to XBlockMixin? That would be more in line with our conventions, wouldn't it (where XBlock and XBlockAside both inherit from XBlockMixin)?

So XBlockMixin is an unfortunate name. "Mixin" in the traditional sense means a class that isn't a meaningful base in itself, but is to be "mixed in" to other classes to add extra functionality. In that sense, the XBlockMixin isn't a mixin; rather, it's supposed to be a base class for new custom XBlock mixins.

Contrast this with Blocklike, which is a meaningful base, one which I expect to be used all over xblock and edx-platform as a type annotation:

def this_function_could_take_a_block_or_an_aisde(block: Blocklike): ...

...which I think reads better than using XBlockMixin as the annotation:

def this_function_could_take_a_block_or_an_aisde(block: XBlockMixin): ...

I guess we could still reconcile the two classes by just aliasing XBlockMixin = Blocklike... what do you think @ormsbee ?

Okay, you've convinced me. Based on your description, XBlockBase would also work for me, but I'm good with Blocklike. I do like the idea of the alias since I have no idea if someone might have built off of that in their fork/plugin.

@ormsbee On second thought, I'm actually inclined to leave XBlockMixin as its own subclass of Blocklike. It can't put my finger on exactly why, but it seems like it'd be less surprising to have XBlockMixin be its own class rather than an alias. Like you say, XBlockMixin is part of the public API, so we can't just delete it.

xblock/core.py

ormsbee · 2024-03-19T04:56:42Z

xblock/test/test_core.py

+    @Blocklike.needs("field-data")
+    class FieldTester(Blocklike):


Can you manually invoke the decorator on Blocklike after it's defined? I haven't done something like this in ages, but I think it'd be something like this?

class Blocklike: # lots of stuff here Blocklike = Blocklike.needs(Blocklike, "field-data")

kdmccormick · 2024-03-19T16:32:07Z

@ormsbee

Can you manually invoke the decorator on Blocklike after it's defined? I haven't done something like this in ages, but I think it'd be something like this?

Good idea. That bit of backwards incompatibility was making me a little nervous, so I'm glad to smooth it out. The syntax ended up being:

Blocklike.needs('field-data')(Blocklike)

kdmccormick mentioned this pull request Feb 12, 2024

[DEPR]: Extraneous XBlock Mixins #714

Closed

kdmccormick force-pushed the kdmccormick/mixins branch 2 times, most recently from 9c06865 to 21ce2ab Compare February 12, 2024 19:00

kdmccormick commented Feb 13, 2024

View reviewed changes

kdmccormick mentioned this pull request Feb 13, 2024

chore: upgrade to XBlock==3.1.0 openedx/edx-platform#34231

Merged

kdmccormick marked this pull request as ready for review February 13, 2024 17:06

kdmccormick mentioned this pull request Feb 15, 2024

refactor: inheritable authoring mixin callbacks for editing & duplication openedx/edx-platform#33756

Merged

kdmccormick force-pushed the kdmccormick/mixins branch from 91c528d to b977536 Compare February 22, 2024 16:24

feanil requested review from salman2013 and farhan February 26, 2024 20:40

kdmccormick force-pushed the kdmccormick/mixins branch 2 times, most recently from cca0fe4 to add3c0e Compare March 18, 2024 16:23

kdmccormick force-pushed the kdmccormick/mixins branch from add3c0e to 51b0920 Compare March 18, 2024 16:32

ormsbee reviewed Mar 19, 2024

View reviewed changes

kdmccormick added 2 commits March 19, 2024 12:14

docs: remove XBlockMixins warning; it's been moved to edx-platform

5990e20

refactor: Blocklike @needs("field-data") directly

d56f410

ormsbee approved these changes Mar 19, 2024

View reviewed changes

kdmccormick merged commit f79bfc5 into openedx:master Mar 19, 2024
10 checks passed

kdmccormick deleted the kdmccormick/mixins branch March 19, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: collapse extraneous XBlock mixins #718

feat!: collapse extraneous XBlock mixins #718

kdmccormick commented Feb 12, 2024 •

edited

Loading

kdmccormick left a comment

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

kdmccormick Feb 13, 2024

ormsbee Mar 19, 2024

kdmccormick Feb 13, 2024

ormsbee left a comment

ormsbee Mar 19, 2024

kdmccormick Mar 19, 2024

kdmccormick Mar 19, 2024

ormsbee Mar 19, 2024

kdmccormick Mar 19, 2024

ormsbee Mar 19, 2024

kdmccormick Mar 19, 2024

ormsbee Mar 19, 2024

kdmccormick commented Mar 19, 2024

		@@ -28,42 +27,3 @@ def __get__(self, instance, owner):


		class_lazy = LazyClassProperty # pylint: disable=invalid-name


		class NamedAttributesMetaclass(type):

		patched_warn.assert_called_once_with("XBlock %s does not contain field %s", type(block), parameter_name)
		patched_warn.assert_called_once_with("%s does not contain field %s", type(block), parameter_name)

		node.set(field_name, text_value)


		class XBlockMixin(Blocklike):

feat!: collapse extraneous XBlock mixins #718

feat!: collapse extraneous XBlock mixins #718

Conversation

kdmccormick commented Feb 12, 2024 • edited Loading

Supporting info

Description

Diagram

Before

After

Change list

Testing

Docs

XBlock and XBlockAside attributes

kdmccormick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented Mar 19, 2024

kdmccormick commented Feb 12, 2024 •

edited

Loading