Skip to content

Commit

Permalink
refactor: make XModuleMixin extend XBlockMixin, not XBlock
Browse files Browse the repository at this point in the history
In order to get typechecking to pass in XBlock, we needed to make
ScorableXBlockMixin a subclass of XBlockMixin. This change, while
sensible, creates an ambiguous method resolution order in the
ProblemBlock, stemming from the fact that XModuleMixin inherits directly
from XBlock.

The fix is to simply make XModuleMixin inherit from XBlockMixin
instead-- this makes sense because XModuleMixin is a helper mixin, not
an XBlock itself. We then must add XBlock as an explicit subclass to all
blocks which inherit from XBlock. This should have no functional impact,
but *should* help us add type hints to the xmodule/ folder in the
future.

Using ProblemBlock as an example... before (XBlock < 6.0.0) we have:

  ProblemBlock <- ScorableXBlockMixin
               <- XModuleMixin <- XBlock <- Blocklike

after (XBlock >= 6.0.0) we have:

  ProblemBlock <- ScorableXBlockMixin <- XBlockMixin <- Blocklike
               <- XModuleMixin        <- XBlockMixin <- Blocklike
               <- XBlock                             <- Blocklike
  • Loading branch information
kdmccormick committed Aug 20, 2024
1 parent 651b624 commit 6c766c7
Show file tree
Hide file tree
Showing 17 changed files with 29 additions and 12 deletions.
1 change: 1 addition & 0 deletions xmodule/annotatable_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class AnnotatableBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
Annotatable XBlock.
Expand Down
1 change: 1 addition & 0 deletions xmodule/capa_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ class ProblemBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
An XBlock representing a "problem".
Expand Down
1 change: 1 addition & 0 deletions xmodule/conditional_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class ConditionalBlock(
ResourceTemplates,
XModuleMixin,
StudioEditableBlock,
XBlock,
):
"""
Blocks child blocks from showing unless certain conditions are met.
Expand Down
1 change: 1 addition & 0 deletions xmodule/error_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class ErrorBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
): # pylint: disable=abstract-method
"""
Block that gets shown to staff when there has been an error while
Expand Down
1 change: 1 addition & 0 deletions xmodule/hidden_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class HiddenBlock(
XmlMixin,
XModuleToXBlockMixin,
XModuleMixin,
XBlock,
):
"""
XBlock class loaded by the runtime when another XBlock type has been disabled
Expand Down
8 changes: 4 additions & 4 deletions xmodule/html_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ def index_dictionary(self):


@edxnotes
class HtmlBlock(HtmlBlockMixin): # lint-amnesty, pylint: disable=abstract-method
class HtmlBlock(HtmlBlockMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
This is the actual HTML XBlock.
Nothing extra is required; this is just a wrapper to include edxnotes support.
Expand All @@ -374,7 +374,7 @@ class AboutFields: # lint-amnesty, pylint: disable=missing-class-docstring


@XBlock.tag("detached")
class AboutBlock(AboutFields, HtmlBlockMixin): # lint-amnesty, pylint: disable=abstract-method
class AboutBlock(AboutFields, HtmlBlockMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
These pieces of course content are treated as HtmlBlocks but we need to overload where the templates are located
in order to be able to create new ones
Expand Down Expand Up @@ -409,7 +409,7 @@ class StaticTabFields:


@XBlock.tag("detached")
class StaticTabBlock(StaticTabFields, HtmlBlockMixin): # lint-amnesty, pylint: disable=abstract-method
class StaticTabBlock(StaticTabFields, HtmlBlockMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
These pieces of course content are treated as HtmlBlocks but we need to overload where the templates are located
in order to be able to create new ones
Expand All @@ -435,7 +435,7 @@ class CourseInfoFields:

@XBlock.tag("detached")
@XBlock.needs('replace_urls')
class CourseInfoBlock(CourseInfoFields, HtmlBlockMixin): # lint-amnesty, pylint: disable=abstract-method
class CourseInfoBlock(CourseInfoFields, HtmlBlockMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
These pieces of course content are treated as HtmlBlock but we need to overload where the templates are located
in order to be able to create new ones
Expand Down
1 change: 1 addition & 0 deletions xmodule/library_content_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class LibraryContentBlock(
ResourceTemplates,
XModuleMixin,
StudioEditableBlock,
XBlock,
):
"""
An XBlock whose children are chosen dynamically from a content library.
Expand Down
1 change: 1 addition & 0 deletions xmodule/lti_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ class LTIBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
): # pylint: disable=abstract-method
"""
THIS MODULE IS DEPRECATED IN FAVOR OF https://github.com/openedx/xblock-lti-consumer
Expand Down
1 change: 1 addition & 0 deletions xmodule/poll_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class PollBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
): # pylint: disable=abstract-method
"""Poll Block"""
# Name of poll to use in links to this poll
Expand Down
2 changes: 2 additions & 0 deletions xmodule/randomize_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.utils.functional import cached_property
from lxml import etree
from web_fragments.fragment import Fragment
from xblock.core import XBlock
from xblock.fields import Integer, Scope
from xmodule.mako_block import MakoTemplateBlockBase
from xmodule.seq_block import SequenceMixin
Expand All @@ -27,6 +28,7 @@ class RandomizeBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
Chooses a random child xblock. Chooses the same one every time for each student.
Expand Down
1 change: 1 addition & 0 deletions xmodule/seq_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ class SequenceBlock(
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
Layout module which lays out content in a temporal sequence
Expand Down
1 change: 1 addition & 0 deletions xmodule/split_test_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class SplitTestBlock( # lint-amnesty, pylint: disable=abstract-method
ResourceTemplates,
XModuleMixin,
StudioEditableBlock,
XBlock,
):
"""
Show the user the appropriate child. Uses the ExperimentState
Expand Down
2 changes: 2 additions & 0 deletions xmodule/template_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class CustomTagTemplateBlock( # pylint: disable=abstract-method
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
A block which provides templates for CustomTagBlock. The template name
Expand Down Expand Up @@ -122,6 +123,7 @@ def export_to_file(self):
class TranslateCustomTagBlock( # pylint: disable=abstract-method
XModuleToXBlockMixin,
XModuleMixin,
XBlock,
):
"""
Converts olx of the form `<$custom_tag attr="" attr=""/>` to CustomTagBlock
Expand Down
5 changes: 3 additions & 2 deletions xmodule/tests/test_xml_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import dateutil.parser

from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
from xblock.core import XBlock
from xblock.field_data import DictFieldData
from xblock.fields import Any, Boolean, Dict, Float, Integer, List, Scope, String
from xblock.runtime import DictKeyValueStore, KvsFieldData
Expand Down Expand Up @@ -65,7 +66,7 @@ class InheritingFieldDataTest(unittest.TestCase):
Tests of InheritingFieldData.
"""

class TestableInheritingXBlock(XmlMixin): # lint-amnesty, pylint: disable=abstract-method
class TestableInheritingXBlock(XmlMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
An XBlock we can use in these tests.
"""
Expand Down Expand Up @@ -227,7 +228,7 @@ def test_defaults_not_inherited_across_lib(self):


class EditableMetadataFieldsTest(unittest.TestCase):
class TestableXmlXBlock(XmlMixin, XModuleMixin): # lint-amnesty, pylint: disable=abstract-method
class TestableXmlXBlock(XmlMixin, XModuleMixin, XBlock): # lint-amnesty, pylint: disable=abstract-method
"""
This is subclassing `XModuleMixin` to use metadata fields in the unmixed class.
"""
Expand Down
3 changes: 2 additions & 1 deletion xmodule/video_block/video_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@
class VideoBlock(
VideoFields, VideoTranscriptsMixin, VideoStudioViewHandlers, VideoStudentViewHandlers,
EmptyDataRawMixin, XmlMixin, EditingMixin, XModuleToXBlockMixin,
ResourceTemplates, XModuleMixin, LicenseMixin):
ResourceTemplates, XModuleMixin, LicenseMixin,
XBlock):
"""
XML source example:
<video show_captions="true"
Expand Down
1 change: 1 addition & 0 deletions xmodule/word_cloud_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class WordCloudBlock( # pylint: disable=abstract-method
XModuleToXBlockMixin,
ResourceTemplates,
XModuleMixin,
XBlock,
):
"""
Word Cloud XBlock.
Expand Down
10 changes: 5 additions & 5 deletions xmodule/x_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from web_fragments.fragment import Fragment
from webob import Response
from webob.multidict import MultiDict
from xblock.core import XBlock, XBlockAside
from xblock.core import XBlock, XBlockAside, XBlockMixin
from xblock.fields import (
Dict,
Float,
Expand Down Expand Up @@ -232,8 +232,8 @@ class XModuleFields:
)


@XBlock.needs("i18n")
class XModuleMixin(XModuleFields, XBlock):
@XBlockMixin.needs("i18n")
class XModuleMixin(XModuleFields, XBlockMixin):
"""
Fields and methods used by XModules internally.
Expand Down Expand Up @@ -736,7 +736,7 @@ def public_view(self, _context):
return Fragment(alert_html.format(display_text))


class XModuleToXBlockMixin:
class XModuleToXBlockMixin(XBlockMixin):
"""
Common code needed by XModule and XBlocks converted from XModules.
"""
Expand All @@ -747,7 +747,7 @@ def ajax_url(self):
"""
return self.runtime.handler_url(self, 'xmodule_handler', '', '').rstrip('/?')

@XBlock.handler
@XBlockMixin.handler
def xmodule_handler(self, request, suffix=None):
"""
XBlock handler that wraps `handle_ajax`
Expand Down

0 comments on commit 6c766c7

Please sign in to comment.