ramonhagenaars / jsons

🐍 A Python lib for (de)serializing Python objects to/from JSON
https://jsons.readthedocs.io
MIT License
289 stars 41 forks source link

Optional and Union fields are not handled correctly #177

Open aquamatthias opened 2 years ago

aquamatthias commented 2 years ago

Thanks for a great helper library. I am not able to use dataclasses in combination with Optional or Union.

Example:

@dataclass
class Foo:
    static: ClassVar[str] = "static"
    a: str
    b: int
    c: Optional[str] = None
    d: Union[str, int] = "test"

def test_roundtrip() -> None:
    foo = Foo("foo", 42, "bar")
    # works
    assert jsons.dump(foo) == {"static": "static", "a": "foo", "b": 42, "c": "bar", "d": "test"}
    # actual {'a': 'foo', 'b': 42}
    assert jsons.dump(foo, strip_class_variables=True) == {"a": "foo", "b": 42, "c": "bar", "d": "test"}

When strip_class_variables is used (which is required in my setup) it does not render Optional or Union types any longer. Any idea how to deal with this?

ramonhagenaars commented 2 years ago

Hi @aquamatthias,

I tested your code and it does indeed not render c and d. What I found however, was that it had nothing to do with Optional or Union, but with there being default values. jsons considers Foo.c and Foo.d as class attributes, because c and d both actually exist on Foo and hold a value:

>>> Foo.d
'test'

Therefore they get skipped when strip_class_variables=True. I'm not so sure what to think of this... technically, jsons is right, because c and d can be accessed on the class. But on a dataclass, it is not unusual to use default values for instance attributes like you do. 🤔

Anyhow, would using strip_attr be of any help in your case?

jsons.dump(foo, strip_attr=("static",))
aquamatthias commented 2 years ago

Hey @ramonhagenaars. Thanks for the explanation - I was unaware of the implication with the default value. I think the workaround is possible for cases where the class variables are known upfront. From a pythonic/library point of view, I still think the test above should work - this is what I would expect without thinking about it.