Documentation: Abstract class can still be instantiated

[c84925]

The documentation makes brief mention that acp.Abstract is different from a class being abstract in the abc sense, but it might still be surprising to users that the following code works:

import abstractcp as acp
import re

class Parser(acp.Abstract):
    PATTERN: str = acp.abstract_class_property(str)

    @classmethod
    def parse(cls, s):
        m = re.fullmatch(cls.PATTERN, s)
        if not m:
            raise ValueError(s)
        return cls(**m.groupdict())

class FooBarParser(Parser):
    PATTERN = r"foo\s+bar"

    def __init__(self):
        super().__init__()

p = Parser()  # OK!
print('p is a ', p)

Of course p.parse("a_string") fails in the re.fullmatch function call. In practice it seems like most users will want to define classes which are both acp.Abstract and abstract in the abc-module sense, so it might be useful to show this explicitly in at least one example, maybe:

import abstractcp as acp
from abc import ABC, abstractmethod
import re

class Parser(acp.Abstract, ABC):
    PATTERN: str = acp.abstract_class_property(str)

    @classmethod
    def parse(cls, s):
        m = re.fullmatch(cls.PATTERN, s)
        if not m:
            raise ValueError(s)
        return cls(**m.groupdict())

    @abstractmethod
    def __init__(self): pass

class FooBarParser(Parser):
    PATTERN = r"foo\s+bar"

    def __init__(self):
        """Make this class concrete."""
        super().__init__()

fbp = FooBarParser()  # OK
p = Parser()  # fails, as intended

[c84925]

Although thinking further, it could be helpful to address this by making acp.Abstract cause a class to be abc-abstract. This could be done by:

• Make acp.Abstract inherit from abc.ABC.
• Make _AbstractClassProperty define a class property __isabstractmethod__ = True.

I think it would be possible to make the setting of __isabstractmethod__ = True non-default to avoid breaking API, selectable with an option to the acp.abstract_class_property function.

[reinhrst]

This is a good point that made me have to think about things for a while. The more I think about it, the more I think you're right: I don't think that there should ever be a reason that one would like to instatiate Parser(), if it has abstract static fields. I think it would be OK to release this as breaking change, but I would need to think about it for a while. Mostly because it should only break code that is kind of broken anyways (you're trying to do dirty stuff I think if you try to instantiate a class with an abstract static property).

Your solution looks quite elegant and works, with 2 caveats:

1. The error is a bit confusing:

    class A(acp.Abstract):
        i = acp.abstract_class_property(int)

    A()

leads to: TypeError: Can't instantiate abstract class A without an implementation for abstract method 'i' -- whereas i is not a method

2. It means that class A(ABC, Abstract):... will not work anymore, since superclasses should be specified so that derived comes before generic (it in here if you're interested, supposedly it didn't change since Python 2.3). This means that the change will actually be more breaking than I initially thought it would be (even in correct code, and the annoying thing is that lots of people may use both ABC and Abstract on the same classes.

[c84925]

Those are really good points and worth taking time to think about the best approach.

For your point (1), I suppose acp.Abstract could define a __new__ function which catches TypeError from its base class and does something with it to be more user-friendly:

class Abstract(ABC):
    def __new__(cls):
        try:
            return super().__new__(cls)
        except TypeError as err:
            # Do something helpful here, although I'm not quite sure what that should be.
            raise

For (2), that is hard also; there could be a transition period where acp.Abstract.__init_subclass__ inspects the __mro__ attribute and, if ABC or ABCMeta appear before acp.Abstract, raise a warning which requests that the caller reorder their inheritance hierarchy to avoid a future error, e.g.:

class Abstract:
    def __init_subclass__(cls, **kwargs):
        if ABC in cls.__mro__ and cls.__mro__.index(ABC) < cls.__mro__.index(Abstract):
            # Warn the caller here about a future breakage, ask them to reorder Abstract before ABC.
        ....

Or to avoid the issue of breaking changes entirely, another option could be to create a new class like acp.AbstractABC where this more-restrictive functionality is implemented, and encourage users to migrate to it for most (all?) use-cases.