gh-141510, PEP 814: Add frozendict support to pickle#144967
gh-141510, PEP 814: Add frozendict support to pickle#144967vstinner wants to merge 8 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
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 |
| 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.
|
|
||
| def _test_recursive_collection_in_key(self, factory, minprotocol=0): | ||
| protocols = range(minprotocol, pickle.HIGHEST_PROTOCOL + 1) | ||
| key = DictKey() |
There was a problem hiding this comment.
We can use Object(), isn't?
| def test_recursive_dict_in_value(self): | ||
| self._test_recursive_collection_in_value(dict) | ||
|
|
||
| def test_recursive_dict_subclass_in_value(self): | ||
| self._test_recursive_collection_in_value(MyDict) |
There was a problem hiding this comment.
This does not add anything in comparison with test_recursive_dict and test_recursive_dict_subclass. The new tests are only needed for immutable frozendict (like there are special tests for tuple and frozenset).
| def test_recursive_dict_in_key(self): | ||
| self._test_recursive_collection_in_key(dict) | ||
|
|
||
| def test_recursive_dict_subclass_in_key(self): | ||
| self._test_recursive_collection_in_key(MyDict) |
There was a problem hiding this comment.
This does not add anything in comparison with more strong test_recursive_dict_key and test_recursive_dict_subclass_key.
Redundant tests make it harder to figure out what particular paths were tested.
Add
frozendict.__getnewargs__()method.