Uh oh!
There was an error while loading. Please reload this page.
gh-141510, PEP 814: Add frozendict support to pickle - #144967
Conversation
Add frozendict.__getnewargs__() method.
Uh oh!
There was an error while loading. Please reload this page.
| # make sure that floats are formatted locale independent with proto 0 | ||
| self.assertEqual(self.dumps(1.2, 0)[0:3], b'F1.') | ||
| def test_frozendict(self): |
There was a problem hiding this comment.
Would not be better to add this test in test_frozendict.py? Together with tests for copy() and deepcopy()?
There was a problem hiding this comment.
Ok, I moved this test to test_pickle.
There was a problem hiding this comment.
Together with tests for copy() and deepcopy()?
Commit dd64e42, which adds frozendict support to the copy module, added frozendict tests to test_copy.
vstinner
commented
Feb 19, 2026
@serhiy-storchaka: I addressed your review. Please review the updated PR. |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
What about frozendict views and iterators? Are they copyable/pickleable?
Are there tests for deepcopying?
I am surprised that there is no separate test_frozendict.py.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka
commented
Feb 19, 2026
Oh, I don't think deepcopy of frozendict is correct. It does not work for recursive frozendict. |
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Feb 19, 2026
I will try to update my PR later to address other comments.
keys, values and items views cannot be copied nor serialized by pickle. Ah, it seems like it's possible to serialize a
test_copy.test_deepcopy_frozendict() tests
Ah. It was simple to add |
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Feb 19, 2026
You're right, the current |
vstinner
commented
Feb 19, 2026
I completed the PR to add requested tests. |
vstinner
commented
Feb 19, 2026
I created #145027 to fix |
| pickle.dumps(fd, proto) | ||
| def test_pickle_iter(self): | ||
| it = iter(frozendict(x=1, y=2)) |
There was a problem hiding this comment.
What about value iterator and item iterator?
Consume one item from the iterator, to ensure that it correctly restores its state. See pickling tests in test_ordered_dict for example.
If things are pickleable, they should also be deepcopyable. It is worth to have explicit deepcopy tests, because they can preserve additional invariants. For example, it should be possible to deepcopy a frozendict containing lambdas or modules.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Feb 20, 2026
I propose merging this PR first (once it will be approved), and then adding such tests to #145027 PR. |
serhiy-storchaka
commented
Feb 20, 2026
Did you forget to push changes for comments which you marked resolved? |
Remove also DictKey (use Object instead).
vstinner
commented
Feb 20, 2026
Ooops, you're correct. I forgot to push my changes, but I also removed my local branch... I had to rewrite my changes. I just pushed them. I should be ok now. |
vstinner
commented
Feb 21, 2026
Merged. Thanks for your great review @serhiy-storchaka, it was very useful! |
StanFromIreland
commented
Feb 21, 2026
This broke Oddballs (i.e. text_xpickle): https://buildbot.python.org/#/builders/1868/builds/5 |
vstinner
commented
Feb 21, 2026
Oh, I'm not used to test_xpickle yet, I forgot about it. I wrote #145069 to fix test_xpickle. |
…4967) Add frozendict.__getnewargs__() method.
…4967) Add frozendict.__getnewargs__() method.
Add
frozendict.__getnewargs__()method.