1

I've recently inherited some code. It has a class called SystemConfig that acts as a grab-bag of constants that are used across the code base. But while a few of the constants are defined directly on that class, a big pile of them are defined as properties of a metaclass of that class. Like this:

class _MetaSystemConfig(type):
    @property
    define CONSTANT_1(cls):
        return "value 1"

    @property
    define CONSTANT_2(cls):
        return "value 2"

    ...

class SystemConfig(metaclass=_MetaSystemConfig):
    CONSTANT_3 = "value 3"
    ...

The class is never instantiated; the values are just used as SystemConfig.CONSTANT_1 and so on.

No-one who is still involved in the project seems to have any idea why it was done this way, except that someone seems to think the guy who did it thought it made unit testing easier.

Can someone explain to me any advantages of doing it this way and why I shouldn't just move all the properties to the SystemConfig class and delete the metaclass?

Edit to add: The metaclass definition doesn't contain anything other than properties.

Tom
  • 7,269
  • 1
  • 42
  • 69
  • 3
    That's really bizarre. There might have been reasons to do it that way, but probably not *good* reasons. Maybe someone wanted to enforce read-only-ness? We can only speculate, though. – user2357112 Nov 15 '22 at 12:31
  • There is probably no way of answering this question that isn't opinion based. @user2357112 has a good idea that they might have really really wanted to enforce that it's "constant". Another possibility is it was intended to work as a mixin class, but then it should just be a normal base in subclasses, and not a metaclass. – Iguananaut Nov 15 '22 at 12:34
  • This is why, although we all hate to do it, it's *critical* to **write freakin documentation**. The previous developer probably thought it was "obvious". There probably isn't any good reason, but we'll never know. – Jared Smith Nov 15 '22 at 12:39
  • @user2357112 - but `_MetaSystemConfig.CONSTANT_1 = "new value"` is really not that much more difficult to write than `SystemConfig.CONSTANT_1 = "new value"` and is just as effective... – Tom Nov 15 '22 at 12:56
  • 1
    Yeah, this just looks grossly overengineered by someone who thought it would be clever to use a metaclass when a class decorator would have sufficed. – chepner Nov 15 '22 at 17:51
  • @chepner worth noting that if you want to be able to add setters to your classmethod properties, then metaclasses are your only option. So it's not entirely insane. – Tom Nov 16 '22 at 13:46
  • I'm disputing the need for properties in the first place here. – chepner Nov 16 '22 at 13:55
  • @chepner there is some sort of sense to it. See my answer but briefly, it means you can chain values so one depends on another. Then if you want to modify them all in one go for unit testing, you only have to patch the one value they all depend on. – Tom Nov 17 '22 at 09:59

2 Answers2

0

So I figured out why it was done this way. These properties were defined as properties because a number of them depended on each other - one for a directory, another for a subdirectory of that directory, several for files spread across the directories and so forth.

But @property doesn't work on classmethods. Python 3.9 fixed @classmethod so that it could be stacked on top of @property but this was removed again in Python 3.11. So, as a workaround, he put the properties in a metaclass (presumably after seeing this question).

However, implementing a property decorator that works on classmethods is not exactly rocket science, so for the good of whoever comes after me and has to figure out what's going on, I've replaced the metaclass properties with class properties on the SystemConfig class. For anyone else who's trying to figure this out, this works as a decorator:

class class_property:
    def __init__(self, _g):
        self._g = _g

    def __get__(_, _, cls):
        return self._g(cls)

Implementing a setter appears to be much more difficult, as __set__ is not used when assigning to class variables. But I don't need it.

Tom
  • 7,269
  • 1
  • 42
  • 69
0

Adding a set of constants to a class can be done with a simple decorator and no properties.

def add_constants(cls):
    cls.CONSTANT_1 = "value 1"
    cls.CONSTANT_2 = "value 2"


@add_constants
class SystemConfig:
    CONSTANT_3 = "value 3"

I'm not concerned about users shooting themselves in the foot by explicitly assigning a new value to any of the "constants", so I consider jumping through hoops just to add read-only class properties more trouble than it's worth.

The problem with metaclasses is that they don't compose. If C1 uses metaclass M1 and C2 uses metaclass M2, you can't assume that class C3(C1, C2): ... will work, because the two metaclasses may not be compatible. The more metaclasses you introduce to do things you could have done without a metaclass, the more problems like this can arise. Use metaclasses when you have no other choice, not just because you think it's a cooler alternative to inheritance or decorators.

chepner
  • 497,756
  • 71
  • 530
  • 681