-
-
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
Change __getstate__ and __setstate__ to use a dict (#1004) #1009
Conversation
6ffbcf6
to
f91ffe4
Compare
Looks like we're missing a branch: https://github.com/python-attrs/attrs/runs/7863540189?check_suite_focus=true#step:6:22 |
I think it’s actually an interesting case missing: what are we gonna do, if there’s extra names? By your motivation, we should actually blow up, no? |
So the case missing is when what we re-hydrate has less attributes than our current class implementation declares, and we have to leave some of them empty. So that's an interesting one. I don't think we should necessarily blow up in every case, as there's some chance calling code can handle this (say, check for I think failing early (on unpickling) is tempting here, but might be too aggressive. It is likely that the calling code that doesn't handle this well and tries to access a non existing attribute will blow up anyway. Another potential solution would be to check if we have an At the end of the day, the point of this is to avoid a clear (if a bit obscure :) ) foot gun, and not re-implement versioning on top of pickle. People who actually need and want that can use protobuf or a number of actual solutions to this problem. So I would go with - just skip it (as it would have before). I will add a test |
Got this locally now so feeling hopeful 🤞 😅 |
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.
Cool, thanks! I'll write a news fragment.
I feel like this may be a breaking change. Do objects serialized with the previous version can be deserialized with the new version? |
They cannot. A fix has been added in #1085 that will be in 23.1, but (de)serializing classes across versions is risky (in general, not just the attrs case) so we'd encourage to look into avoiding that. |
Summary
We use a dict instead of a tuple, so that we assign only existing attributes on unpickle, if the class
definition has changed in the meantime.
Resolves #1004