daffidwilde / matching

A package for solving matching games
https://daffidwilde.github.io/matching/
MIT License
150 stars 42 forks source link

Should `matching.Game` be a metaclass? #90

Closed daffidwilde closed 4 years ago

daffidwilde commented 4 years ago

On the review, @igarizio raised the following point about matching.Game:

On game.py, you mention that the Game class is just a “base class and is not intended for use other than inheritance”. Maybe you can use a metaclass to highlight that (so users get a nice warning on their IDE when subclassing it). This is just a suggestion, your code in completely fine without it.

I was wondering how this might be done. My initial attempt has been to let matching.Game inherit from type but that would mean a large refactor of the inheritance for the rest of the games since a metaclass has its own requisites beyond a class.

Is there a simpler way that I'm totally missing? I agree it'd be nice to give users some feedback when using the library to implement new games but don't want to cloud the architecture too much.

igarizio commented 4 years ago

@daffidwilde Thank you for taking the time to consider the suggestion! My go-to solution for metaclasses is through abc (part of the standard library). Here is how Game would look like using abc:

import abc

class BaseGame(metaclass=abc.ABCMeta):  # Renamed just to make it more clear.

    def __init__(self):
        self._matching = None
        self.blocking_pairs = None

    @property
    def matching(self):
        """ Property method to stop direct write access. """
        return self._matching

    @matching.getter
    def matching(self):
        """ Property method to stop direct write access. """
        return self._matching

    @abc.abstractmethod
    def solve(self):
        """ Placeholder for solving the given matching game. """
        pass

    @abc.abstractmethod
    def check_stability(self):
        """ Placeholder for checking the stability of the current matching. """
        pass

    @abc.abstractmethod
    def check_validity(self):
        """ Placeholder for checking the validity of the current matching. """
        pass

Now, if I try to instantiate BaseGame I get TypeError: Can't instantiate abstract class BaseGame with abstract methods.... Also, if I subclass it without implementing the required methods, my IDE (Pycharm) says "Class Dummy must implement all abstract methods". Finally, all tests pass, except for the ones on test_game.py, which is expected since it's not possible to instantiate Game anymore. (If I may suggest a way rewrite those tests, I would say that creating a dummy subclass and test the parameters there, would be a good idea.)

daffidwilde commented 4 years ago

That is great -- thanks so much for that @igarizio.