ENH correctly restore default_factory of a defaultdict#433
ENH correctly restore default_factory of a defaultdict#433adrinjalali merged 7 commits intoskops-dev:mainfrom
Conversation
BenjaminBossan
left a comment
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for fixing this bug. I added a few comments, please take a look.
|
|
||
| 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)])
BenjaminBossan
left a comment
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for fixing this, LGTM.
Failing CI seems to be unrelated, some tests fail because of a change in sklearn and some because MacOS is no longer supported by codecov (??). So feel free to merge despite the red CI.
|
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. |
3 participants
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