Add type hints and check them in CI

[kdmccormick]

XBlock is a widely used core library, but is pretty abstract and thus can be hard to reason about. So, this would be a particularly valuable repo to add type hinting to.

Some additional notes are here:

• https://github.com/openedx/XBlock/issues/710

Draft PR here:

• https://github.com/openedx/XBlock/pull/761

[kdmccormick]

early WIP: https://github.com/openedx/XBlock/pull/713

[kdmccormick]

@bradenmacdonald I've been working on this, and it's going pretty well. I'm actually really impressed how far mypy has come--typing XBlock Fields as generic descriptors works great!

The hardest part so far has been dealing with how XBlock's implementation is chopped up into a bunch of tiny mixins. I guess that was in anticipation of supporting a bunch of XBlock-like classes that don't use all of XBlock's capability, but AFAICT we only have XBlock and XBlockAside. A good demonstration of the pain is HierarchyMixin, which is written in such a way that I can't see it ever working outside of the XBlock class. Here's the typing hack I'm using to get around it.

I'm really tempted to collapse nearly all the mixins down into a simple hierarchy:

class Plugin:
    ... # as is
class ScopedStorageMixin:
    ... # collapse in: RuntimeServicesMixin
        # could be better named: RuntimeAndStorageMixin

class XBlockMixin(ScopedStorageMixin):
    ... # as is

class SharedBlockBase(ScopedStorageMixin, Plugin):
    ... # collapse in: XmlSerializationMixin, HandlersMixin

class XBlockAside(SharedBlockBase):
    ... # as is

class XBlock(SharedBlockBase):
    ... # collapse in: HierarchyMixin, ViewsMixin

Does that sound like a good move? Or do you think the current set of mixins is worth keeping around?

[kdmccormick]

Alternatively, here's an even more aggressive simplification. It would add capability to XBlockMixin, but I honestly think we'll need that extra capability when we go to apply type-checking to the XBlockMixins in edx-platform:

class Plugin:
   ... # as-is
class Blocklike:
   ... # collapse in: RuntimerServicesMixin, ScopedStorageMixin, XmlSerializationMixin, HandlersMixin

class XBlockMixin(Blocklike):
   ... # as is

class XBlockAside(Blocklike, Plugin):
   ... # as is

class XBlock(Blocklike, Plugin):
   ... # collapse in: HierarchyMixin, ViewMixin

[bradenmacdonald]

@kdmccormick I think that the only useful aspect of the current mixins is code organization - grouping similar functions, attributes, etc. together in their own files. I don't think they're actually used separately in any meaningful way. So I'm totally in favor of some simplifications.

In fact, the current organization often makes it hard for me to find code when I need to look something up, if I can't remember which mixin it's in.

[ashultz0]

Having been in those mixins in specific trying to understand how xblock asides work, I could not be more in favor of collapsing them together.

And In general the code uses way too many mixins so if there's an opportunity to lower that with no side effects that's awesome.

[kdmccormick]

Great! I opened:

• https://github.com/openedx/XBlock/issues/714

I will probably open a separate PR for the mixin collapsing, which I'll rebase #713 onto.

[kdmccormick]

This is almost ready or review: https://github.com/openedx/XBlock/pull/761