gh-141510, PEP 814: Add frozendict support to pickle#144967
gh-141510, PEP 814: Add frozendict support to pickle#144967vstinner wants to merge 6 commits intopython:mainfrom
Conversation
Add frozendict.__getnewargs__() method.
Lib/test/pickletester.py
Outdated
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. |
| def test_recursive_dict_and_inst(self): | ||
| self._test_recursive_collection_and_inst(dict.fromkeys, oldminproto=0) | ||
|
|
||
| def test_recursive_frozendict_and_inst(self): |
There was a problem hiding this comment.
Please add tests for frozendict subclass, like test_recursive_frozenset_subclass_and_inst.
Please add tests for different types of recursion:
- frozendict -> mutable value (list) -> frozendict
- frozendict -> mutable key -> frozendict
All tests should be repeated for a frozendict subclass.
|
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 |
| p = pickle.dumps(fd, proto) | ||
| fd2 = pickle.loads(p) | ||
| self.assertEqual(fd2, fd) | ||
| self.assertEqual(type(fd2), type(fd)) |
There was a problem hiding this comment.
For FrozenDictSlots, test also the slot value (non-default). And maybe the value of a non-slot attribute, For FrozenDict, also test an attribute value.
2 participants
Add
frozendict.__getnewargs__()method.