gh-141510, PEP 814: Add frozendict support to pickle#144967
gh-141510, PEP 814: Add frozendict support to pickle#144967vstinner wants to merge 10 commits intopython:mainfrom
Conversation
Add frozendict.__getnewargs__() method.
Lib/test/pickletester.py
Outdated
| # 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.
|
@serhiy-storchaka: I addressed your review. Please review the updated PR. |
serhiy-storchaka
left a comment
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.
|
Oh, I don't think deepcopy of frozendict is correct. It does not work for recursive frozendict. |
|
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 |
You're right, the current |
|
I completed the PR to add requested tests. |
I created #145027 to fix |
Lib/test/test_dict.py
Outdated
| 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.
I propose merging this PR first (once it will be approved), and then adding such tests to #145027 PR. |
|
Did you forget to push changes for comments which you marked resolved? |
Remove also DictKey (use Object instead).
|
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. |
2 participants
Add
frozendict.__getnewargs__()method.