Bug: Sortinfo inheritance

[emm68]

pydmrs/pydmrs/core.py

Line 145 in 6d24bda

elif isinstance(sortinfo, Sortinfo): # Allow Sortinfo instances

If sortinfo is of type EventSortinfo or InstanceSortinfo, the check fails and we end up with PydmrsTypeError: unsupported type for sortinfo. Something is wrong with how the inheritance is set up.

This shows up when you're custom building a Node from scratch, I haven't encountered it in other scenarios.

[goodmami]

While I'm not certain, this could be due to Sortinfo using a metaclass. There's some documentation about defining __subclasshook__ that seems relevant.

[guyemerson]

Perhaps it would be a good time to refactor the Sortinfo code using __init_subclass__, which will make the code easier to read and avoid bugs like this. This would require Python 3.6, but that was released almost three years ago now, and 3.5 will be reaching end-of-life in just over a year.

[guyemerson]

Having said that, I can't recreate this bug.

>>> import pydmrs.core
>>> n = pydmrs.core.Node(sortinfo=pydmrs.components.EventSortinfo())

I get no errors in the above code. And checking more directly:

>>> import pydmrs.components
>>> e = pydmrs.components.EventSortinfo()
>>> isinstance(e, pydmrs.components.Sortinfo)
True
>>> x = pydmrs.components.InstanceSortinfo()
>>> isinstance(x, pydmrs.components.Sortinfo)
True

[goodmami]

Although I wonder why something like SortInfo needs metaclasses at all. It seems the motivation is to allow features to be class attributes (sortinfo.sf, etc.), but there are other ways to do this (like overriding getattr()), and also it is not robust to grammars that define properties that are not valid identifiers, like gg's MASS-UNIT or COG-ST of Matrix grammars.

[guyemerson]

I used a metaclass with other grammars in mind -- I wanted to make it as easy as possible for users to specify features in their own subclasses, e.g.

class MatrixInstanceSortinfo(InstanceSortinfo):
    __slots__ = ('cog_st',)

Although I will admit I haven't documented this properly, other than a comment in the code that says "Users can define subclasses with additional features".