ENH correctly restore default_factory of a defaultdict #433
Conversation
skops/io/_trusted_types.py
Outdated
| @@ -10,6 +10,10 @@ | |||
|
|
|||
| PRIMITIVE_TYPE_NAMES = ["builtins." + t.__name__ for t in PRIMITIVES_TYPES] | |||
|
|
|||
| BUILTIN_TYPES = [list, set, map, tuple] | |||
There was a problem hiding this comment.
I wonder about the name. Above we have "primitive types" but they are also "builtin". Should this be "complex types", "container types", "composite types" or something along these lines?
Then we could have BUILTIN_TYPE_NAMES = PRIMITIVE_TYPE_NAMES + COMPLEX_TYPE_NAMES (or whatever name is chosen) and save a bit of code duplication.
| obj["foo"] = "bar" | ||
| obj_loaded = loads(dumps(obj)) | ||
| assert obj_loaded == obj | ||
| assert obj_loaded.default_factory == obj.default_factory |
There was a problem hiding this comment.
Should a test for OrderedDict be added too?
|
@BenjaminBossan thanks for the review, I think I addressed your comments |
| def test_dictionary(cls): | ||
| obj = cls({1: 5, 6: 3, 2: 4}) | ||
| loaded_obj = loads(dumps(obj)) | ||
| assert obj == loaded_obj |
There was a problem hiding this comment.
Should the type also be checked? E.g. this passes:
assert {2: 20, 1: 10} == OrderedDict([(2, 20), (1, 10)])
assert {1: 10, 2: 20} == OrderedDict([(2, 20), (1, 10)])There was a problem hiding this comment.
oops, sorry. Added now @BenjaminBossan
|
Do you have an ETA for a release which would include this fix? |
|
Working on some sklearn 1.6 compatibility issues (due to come out soon) and then we'd do a release. |
Fixes #432
During the inspection of #432 we also realized
default_factoryis not restored.This also adds
defaultdict, and a few other primitive builtin types as trusted.cc @BenjaminBossan