attrs icon indicating copy to clipboard operation
attrs copied to clipboard

Suggestion: Consider changing the order for variable initialization in `__init__`

Open germaniuss opened this issue 1 year ago • 1 comments

First of all thanks a lot for the awesome library! I just recently discovered it and it saved me so many headaches. Recently I was trying to do something of this sort, where either x or y has to be given to the __init__ method for this to be valid:

@define
class MyClass:
    x: int = field()
    y: int = field()
    
    @x.default
    def y_plus_one(self):
        return self.y + 1
        
    @y.default
    def x_plus_one(self):
        return self.x + 1

This however (as the docs state) fails because self.y would not be initialized during the call to y_plus_one. Even though the docs state this fails I wanted to try and fix the issue and managed to do so in a very hacky way. However in the process I thought up a non invasive way in which this may work.

If we first initialize those fields that were given a value through __init__ and then use the NOTHING sentinel to initialize the values for those attributes that were not initialized through __init__ but before calling the factories, the problem dissapears. The reordered __init__ method would look something like this (I'm not really sure how the method is autogenerated but its a rough idea):

@define
class MyClass:
    ...
    
    def __init__(self, x = NOTHING, y = NOTHING): # <- this is autogenerated
        # initialize all fields before calling factories
        if x is not NOTHING:
            self.x = x
        elif isinstance(self.__attrs_attrs__['x'].default, Factory):
            self.x = NOTHING
        elif self.__attrs_attrs__['x'].default is not None:
            self.x = self.__attrs_attrs__['x'].default

        if y is not NOTHING:
            self.y = y
        elif isinstance(self.__attrs_attrs__['y'].default, Factory):
            self.y = NOTHING
        elif self.__attrs_attrs__['y'].default is not None:
            self.y = self.__attrs_attrs__['y'].default

        # call the factories for all fields that were not given a value through init
        if self.x is NOTHING and isinstance(self.__attrs_attrs__['x'].default, Factory):
            self.x = self.__attrs_attrs__['x'].default.factory(self)
            if self.x is NOTHING:
                raise ValueError("a factory may not return the NOTHING sentinel")

        if self.y is NOTHING and isinstance(self.__attrs_attrs__['y'].default, Factory):
            self.y = self.__attrs_attrs__['y'].default.factory(self)
            if self.y is NOTHING:
                raise ValueError("a factory may not return the NOTHING sentinel")
    
    ...

This has the additional benefit that now I can throw more descriptive error messages during initialization, like:

    ...
    @x.default
    def y_plus_one(self):
        if self.y is NOTHING:
            raise ValueError("either x or y has to be initialized")
        return self.y + 1
    ...

germaniuss avatar Jun 14 '24 21:06 germaniuss

I've written a patch locally and it works, so I'd be willing to send a PR if you're interested.

germaniuss avatar Jun 15 '24 13:06 germaniuss