-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support custom __getstate__, __setstate__ for slotted classes #513
Support custom __getstate__, __setstate__ for slotted classes #513
Conversation
I’m pretty sure this breaks with subclassing? I’ve had the idea of “auto detecting” implementations and implicitly switching them off for a while and would love it for all features but so far nobody came up a with a solid implementation. I guess what i’d Like is a There might be even an open PR where someone tried and didn’t quite follow thru. |
What exactly do you mean "breaks with subclassing"? @hynek I'd claim that everything works as expected. Note that this fix is not about an There is no magic solution for custom It is a great feature that Regular classes:
Same example for (non-slotted)
Why should not the same be possible for slotted-classes? |
A nice
But this is orthogonal to the fix in this PR, which would still be relevant I think? (I have a PoC implementation of the example, and this is maybe along the lines of your work-in-progress API additions?) Edit: this feature should of course work the same way for slotted classes. |
To be clear: the thing you want is swell, and I want it for all methods! But the use-case I mean is this: @attr.s(slots=True, auto_attribs=True)
class Base:
a: int
def __getstate__(self):
pass # does whatever
def __setstate__(self, state):
pass # does whatever
@attr.s(slots=True, auto_attribs=True)
class C(Base):
b: int I'm fairly certain that your code is going to "detect" Base's methods while creating C? |
Exactly; when creating The only reason (I can think of) for implementing the methods in
It's your responsibility to "continue the custom implementation path" once you've started it. What would you say is the wanted behaviour for pickleing of |
…s for subclassing
@@ -529,3 +529,144 @@ class C(object): | |||
w = weakref.ref(c) | |||
|
|||
assert c is w() | |||
|
|||
|
|||
def _loads_dumps(instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO test all protocols
Actually, I was wrong about the behaviour of this PR:s fix in the previous comment; in fact, it didn't "detect" Base's methods while creating C?" (which I claim is wanted behaviour). The latest commit fixes this though. I added several test cases (there was no previous coverage of these auto-generated methods), the following (subset) would fail on master (all in If you get the time, please let me know which of these that shouldn't pass and why, and any new test case that fails with the fix in this PR. I'm sorry if I misunderstand your concern/objection! Not sure I understand the "want it for all methods" part, but I think this can be viewed as an isolated current shortcoming - that can be fixed 😄 |
So I disagree on the subclass detection thing, because you don't want this stuff to fall apart, just because some base class of yours adds a getstate. All that said, I think the best approach here to fix it for now is to follow our existing precedent: Wouldn't something like |
Maybe we just disagree - but I still think that you’re missing the point:
But again, My tl;dr is this:
|
I'm starting to suspect that we don't disagree and I'm just confused. The docs say:
Isn't that entirely wrong and those methods are added always for slotted classes currently? |
Well, maybe not wrong (strictly speaking) but certainly incomplete/confusing since yes; these methods are always added(/overridden) for So let's clarify some things: it is the docs that are misleading here,
So the current implementation is right in that it adds the methods only based on |
Also, to recap: my initial issue had nothing to do with subclassing, the problem was simply that my custom If a class, slotted or not, implements Agree? That being said, this is the first time I'm poking around in the internals of attrs so I'm not trying to argue the quality of this PR - just want to reach consensus and understanding of what the wanted behaviour is. |
This clarifies current behaviour: #521 |
I still disagree on the inheritance thing, because I just don't think that attrs should behave differently based on something the user can't see because it came from from some random base class they didn't even know about. So far, attrs never did anything like that (but read on ;)) I'm gonna give you things I plan to do and you tell me, if that solves your problems, okay?
How does that sound to you? |
I regret coming across as stubborn, but I don't see how we're getting closer to defining what the wanted behaviour is and why it is desired that it differs between non-attrs-classes and non-slotted-attrs-classes, on one side, vs. slotted-attrs-classes on the other. I just feel like repeating my previous arguments and don't see them being responded to in principle:
I really can't relate to your view of inheritance in:
I'm no fan of inheritance per se and think that it can often lead to unnecessary complexity, but who the h*!l goes off inheriting random base classes they don't even know about!? To your question/suggestion: This is not a blocker in any way. In the very rare cases that I'd need a custom Again, sorry for being stubborn. I promise not to argue further without more elaborate counter-arguments ;) |
Don't worry about stubbornness, I very much appreciate you taking the time dancing with me in circles. :) Let's start simple:
The answer is frameworks and third party libraries. Number 2 shouldn't be in direct conflict with what you want, it just doesn't go far enough. Eventually, what you're supposed to be able to write is just: @attrs.define
class C:
x: int
def __setstate__(self, state):
...
def __getstate__(self):
...
I mean I agree; the only reason we're doing that is because it's impossible to add more stuff to a slotted class. I wonder if we shouldn't consider to write The ultimate question and the core conflict with our approaches is: why do you think I find especially pickle methods unfit for it, because I'd assume that some class you're subclasing might write some pickle method for itself and the moment you try to expand it by subclassing, those methods stop being adequate. Or am I missing anything here? |
Thanks for a more elaborate response ;)
I see your concern (I intentionally interpreted your formulation rather literal). But if you are on a mission to protect child classes from their base ditto, then that should go for non-slotted as well. So this argument is only consistent if you consider doing it for all attrs classes - which I see you might do...
Not sure I follow here, isn't the only(/original) reason
No, I'm prepared to loosen my position here a bit ;) I think it is great with a solution that avoids overriding these methods when they are declared as part of the class definition in question, and fine (!) if it still does override if they are present by inheritance. Principally, I withstand that there is no need to differentiate between these cases, but in practice, it makes no difference:
Preferably this should be the default behaviour and not require some
Well, they are already obviously special as of now: attrs only autogenerate/override them in slotted classes to support pickling of these with old protocols ...and this autogeneration/overriding is not needed if they already implement the methods. But sure, if the actual motivation is "protect from changes in base classes" and if it is applied to all classes, then they are no longer special. As to the "core conflict with our approaches" - and to go further meta ;) - I think that you directly integrated my issue under the
Here you seem to make the same observation as I do in "2" above but drawing different conclusions. However, I think we agree that your second solution obtains good behaviour in practice. |
Since I've finally merged #607 (in my defense, nobody wanted to review…), wouldn't it make sense to extend it to operate on |
Cool, I won't have time to look at it in-depth in the near future, but from the look of it yes 👍 |
Fixed by #642 |
As per #512 you currently can not implement your custom
__getstate__
__setstate__
methods on slotted classes. With this change, attrs does not automatically create and override these methods if they are already implemented - thus making the behaviour consistent with the slotted classes documentation (as far as I understand!?):Edit: I read the docs more carefully and now understand that "You can support protocol 0 and 1 by implementing
__getstate__
and__setstate__
methods yourself" refers to__slots__
classes in general but not toattr.s(slots=True)
classes. I still don't see any downside from making it possible to define these methods inattr.s(slots=True)
classes.Pull Request Check List
.pyi
).tests/typing_example.py
.docs/api.rst
by hand.@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives..rst
files is written using semantic newlines.changelog.d
.