biqqles / dataclassy

A fast and flexible reimplementation of data classes
https://pypi.org/project/dataclassy
Mozilla Public License 2.0
81 stars 9 forks source link

Calling user defined init when init=False doesn't pass args through #8

Closed kgreav closed 3 years ago

kgreav commented 3 years ago

Great work on this lib so far. Much more intuitive than dataclasses and even solves some bugs (e.g in dataclasses you can't define a post_init parameter with the same name as a method on the class).

The only issue I've run into so far is that if you specify init=False but you have a custom __init__, you can't create your class with positional arguments, and you have to always specify keyword arguments. Seems to me that we should just pass all arguments straight through to the user defined __init__ when init=False, and just call __new__ with no args.

I've done a PR (#9) that fixes this issue I think without breaking any existing usage, but keen to get your thoughts.

biqqles commented 3 years ago

Hi, thanks for this report. I wonder if it wouldn't it be enough to simply change line 85 to something like

return type.__new__(DataClassInit if options['init'] and user_init else mcs, name, bases, dict_)

This is optimal because it avoids the definition of a slow Python __call__.

Apologies, I am away for exams and will likely not be able to test and/or merge for a while.

kgreav commented 3 years ago

No worries and no rush, I'll test it out - already set up everything in my repo so I'm not beholden to the PR being merged.

kgreav commented 3 years ago

OK, after testing your solution is equivalent to mine in function but optimal, however I realised there is one breaking change (applicable to both versions). Personally I don't think it is a bad breaking change as the usage was quite misleading.

In the existing, the following works:

@dataclass(init=False)
class Obj(object):
    a: str
    b: str

    def __init__(self, c, d):
        self.a = c
        self.b = d

o = Obj("x", "y", "w", "z")
assert o.a = "w"
assert o.b = "z"

whereas in the new version this will fail, and would need to be changed to:

Obj("w", "z")

This is obviously a naive example, and someone could have more contrived examples that make sense, however the worst case would be they'd have to add their classes attributes to their __init__ function which I think is desirable anyway, as from what I've seen you can't currently get a meaningful constructor signature out of this example unless you document it, as it's an amalgamation of __new__ and __init__.

biqqles commented 3 years ago

I think it's fair to call the behaviour you discovered a bug - init is false and yet the arguments its function would have had are still being consumed. This was an oversight on my part - there is absolutely no reason to use a custom __call__ to modify arguments if there isn't both __init__ and __new__ on the class. If someone is using this obscure behaviour, for some reason, it is a breaking change - but we're still on 0.x releases so what can they expect!

biqqles commented 3 years ago

I took a few moments to push the fix for this. Any problems, just let me know.

biqqles commented 3 years ago

Fix released in v0.6.2. Also, apologies, I didn't realise you had incorporated my solution into your PR before I unceremoniously closed it.

kgreav commented 3 years ago

Not a problem at all, the quick turnaround more than makes up for it - I didn't get around to fixing/writing tests anyway. Thanks a bunch!