Skip to content

kw_only tuple support? #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
henryiii opened this issue Mar 15, 2022 · 12 comments
Open

kw_only tuple support? #234

henryiii opened this issue Mar 15, 2022 · 12 comments

Comments

@henryiii
Copy link
Contributor

  • cattrs version: 1.10.0
  • Python version: 3.10.2
  • Operating System: macOS

Description

I have a nested data structure using attrs and kw_only. Converting to a tuple works beautifully, letting me load this structure into a SQLite3 database.

import attrs
import cattrs

@attrs.define(kw_only=True)
class A:
    a: str = ""

@attrs.define(kw_only=True)
class B(A):
    b: int

# PS: The reason for kw_only is primarily because it makes subclassing work properly
# for building data structures with arbitrary defaults - the above requires
# kw_only in either attrs or Python 3.10's dataclasses.

converter = cattrs.GenConverter()
converter.unstructure_attrs_astuple(B(b=2)) # -> ('', 2)

However, the other direction fails:

>>> converter.structure_attrs_fromtuple(('', 2), B)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/henryschreiner/git/CMS/hypernewsviewer/.venv/lib/python3.10/site-packages/cattr/converters.py", line 424, in structure_attrs_fromtuple
    return cl(*conv_obj)  # type: ignore
TypeError: B.__init__() takes 1 positional argument but 3 were given

My current workaround is a little ugly and possibly not very efficient:

>>> converter.structure_attrs_fromdict({n.name:i for n,i in zip(attrs.fields(B), ('', 2))}, B)
B(a='', b=2) 

Would it be possible to make fromtuple work on a kw_only class, like astuple does? IMO, kw_only is about subclassing & controlling manual creation, not raw conversion.

Might be hard/impossible, but thought I'd ask.

@Tinche
Copy link
Member

Tinche commented Mar 16, 2022

I don't think it'd be difficult at all, I'll put it on my radar.

In the meantime, here's how you can solve the issue yourself:

from attrs import fields, has
from cattrs import GenConverter

c = GenConverter()


def structure_kw_attrs_fromtuple(obj, cls):
    conv_obj = {}
    for a, value in zip(fields(cls), obj):
        # We detect the type by the metadata.
        converted = c._structure_attribute(a, value)
        conv_obj[a.name] = converted

    return cls(**conv_obj)


c.register_structure_hook_func(
    lambda t: has(t) and any(a.kw_only for a in fields(t)),
    structure_kw_attrs_fromtuple,
)
>>> c.structure(('', 2), B)
B(a='', b=2)

@henryiii
Copy link
Contributor Author

Hmm, using this produces ValueError: 'previous_num' is not a valid ContentType (that's an enum). previous_num is not a ContentType, though, but an Optional[int], so the ordering here is muddled, or it's triggering incorrectly.

Ahaha, it's running when I'm trying to convert from a dict. Is there a way to make this tuple only? Or should I just implement the dict process here too?

@Tinche
Copy link
Member

Tinche commented Mar 16, 2022

Ah, the hook I gave you will run automatically for any attrs class that has at least one kw arg.

If you don't need it to run automatically, skip register_structure_hook_func and just call it directly (structure_kw_attrs_fromtuple(('', 2), B)).

@Tinche
Copy link
Member

Tinche commented Mar 16, 2022

Don't see anything immediately wrong with my snippet, can you prepare a small sample?

@henryiii
Copy link
Contributor Author

I'd be fine with it running automatically, but I want to be able to support dicts and tuples. (That is, converter.structure_attrs_fromtuple and converter.structure (or converter.structure_attrs_fromdict ) would ideally both work). Adding the above causes the dict input to start failing.

@henryiii
Copy link
Contributor Author

converter.structure_attrs_fromdict does still work. (And this doesn't change converter.structure_attrs_fromtuple). Okay, that's enough to work with, I think. I could make convert be the tuple one, and use the fromdict one for the dict.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 16, 2022

(And here's the requested sample to see what I was thinking)

import attrs
import cattrs

@attrs.define(kw_only=True)
class A:
    a: str = ""

@attrs.define(kw_only=True)
class B(A):
    b: int


converter = cattrs.GenConverter()

def structure_kw_attrs_fromtuple(obj, cls):
    conv_obj = {}
    for a, value in zip(attrs.fields(cls), obj):
        # We detect the type by the metadata.
        converted = converter._structure_attribute(a, value)
        conv_obj[a.name] = converted

    return cls(**conv_obj)


converter.register_structure_hook_func(
    lambda t: attrs.has(t) and any(a.kw_only for a in attrs.fields(t)),
    structure_kw_attrs_fromtuple,
)

print(converter.structure_attrs_fromdict({"a": "hi", "b": 2}, B))
# print(converter.structure({"a": "hi", "b": 2}, B))  # Breaks when the above is added

# converter.structure_attrs_fromtuple(('', 2), B)  # Doesn't work when adding the above... or before adding it :)
print(converter.structure(('', 2), B))

@Tinche
Copy link
Member

Tinche commented Mar 16, 2022

Do you need to support dicts and tuples in the same payload? If not, you can just have two converters. If yes, it gets a little complicated but can still be done.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 16, 2022

Do you need to support dicts and tuples in the same payload?

No, I'd be fine with only supporting structure_attrs_fromdict & structure_attrs_fromtuple/structure and being explicit. Reading from the database is always a tuple, reading from the original un-databased file is a dict. So this is enough to make it work by changing my dict structure to fromdict then using this as the "default" converter.

@henryiii
Copy link
Contributor Author

Just curious, can I copy an existing converter (with copy.copy or something) and make a few more modifications? If I went with two converters, I'd want most of the initial setup the same.

@Tinche
Copy link
Member

Tinche commented Mar 16, 2022

I would just instantiate the GenConverter twice, and apply the tuple tweak to just one. They are cheap and the intended design pattern is to have one per serialization method (so you might have one for data coming from web/json, another one for data from a database, a third one for yaml from a file, etc).

Just curious, can I copy an existing converter (with copy.copy or something) and make a few more modifications? If I went with two converters, I'd want most of the initial setup the same.

You can't right now. I've had people request it though, so it's on the roadmap. It'll probably be a .copy() method.

@henryiii
Copy link
Contributor Author

Thanks! Couldn't you implement __copy__ then have a .copy() shortcut? Nice to support the "standard" way (though you'd have to ensure you aren't copying them already and depending on the default implementation, but I'd assume not if it's not supported).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants