python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
815 stars 113 forks source link

Adding a `make_dict_unstructure_fn` breaks `omit_if_default=True` #238

Open MicaelJarniac opened 2 years ago

MicaelJarniac commented 2 years ago

Description

I'm yet to get a minimal working example, but it's something like so:

@attrs.define
class Whatever:
    foo: str = "foo"
    bar: str = "bar"
    thing: str = "thing"
    u_thing: str = "_thing"

whatever = Whatever(bar="baz")

c = cattrs.GenConverter(omit_if_default=True)

c.unstructure(whatever)  # Does omit if default!

unstruct_hook = cattr.gen.make_dict_unstructure_fn(
    Whatever,
    c,
    **{"u_thing": cattr.override(rename="_thing")}
)
c.register_unstructure_hook(Whatever, unstruct_hook)

c.unstructure(whatever)  # Does NOT omit if default :c

I do a lot of other things to the converters, but from my testing, this is what's breaking it.

I need those extra hooks because I need both thing and _thing to show up on the output, and attrs doesn't seem to allow that, so I need to rename it on the fly.

MicaelJarniac commented 2 years ago

Sort of related, but not quite: https://github.com/python-attrs/attrs/issues/391

Tinche commented 2 years ago

So when you pass omit_if_default=True to the GenConverter, it'll use it when it calls make_dict_unstructure_fn for you. But if you're calling it yourself, you need to pass it in yourself too. So do:

unstruct_hook = cattr.gen.make_dict_unstructure_fn(
    Whatever,
    c,
    u_thing=cattr.override(rename="_thing"), _cattrs_omit_if_default=True
)
c.register_unstructure_hook(Whatever, unstruct_hook)

The parameter name is a little mangled to not clash with a possible attribute of that name.

We could make it so that the converter behavior is used if the flag is not set explicitly. If you feel like putting together a PR for this, I'd be happy to merge it in ;)