Skip to content
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

Respect type annotation for containers #129

Closed
Dr-ZeeD opened this issue Mar 9, 2021 · 14 comments
Closed

Respect type annotation for containers #129

Dr-ZeeD opened this issue Mar 9, 2021 · 14 comments

Comments

@Dr-ZeeD
Copy link

Dr-ZeeD commented Mar 9, 2021

  • cattrs version: 1.3.0
  • Python version: Python 3.9.1
  • Operating System: macOS 11.2.1

Description

If I see it correctly, support for generics exist. However, containers can also be generic and in combination with Protocols we could do neat things.

In a way, this leads me to a question: Why do unstructure hooks not also take an optional cls argument with the annotated type?

What I Did

from collections.abc import MutableSequence
from typing import Any, Protocol

from attr import define
from cattr import GenConverter

converter = GenConverter()


class A(Protocol):
    a: int


@define
class B:
    a: int
    b: int


def is_A(cls: type) -> bool:
    return cls is A


def default_unstructure_with_type(data: type[A]) -> dict[Any, Any]:
    return {"_type": type(data).__qualname__} | converter.unstructure(data)


converter.register_unstructure_hook_func(is_A, default_unstructure_with_type)
# using a registry we might also supply a structure function that acts on "_type"


if __name__ == "__main__":
    b = [B(1, 2), B(3, 4)]

    print(converter.unstructure(b[0]))
    # -> {'a': 1, 'b': 2}
    # OK

    print(converter.unstructure(b[0], unstructure_as=A))
    # -> {'_type': 'B', 'a': 1, 'b': 2}
    # OK

    print(converter.unstructure(b))
    # -> [{'a': 1, 'b': 2}, {'a': 3, 'b': 4}]
    # OK

    print(converter.unstructure(b, unstructure_as=MutableSequence[A]))
    # -> [{'a': 1, 'b': 2}, {'a': 3, 'b': 4}]
    # Not OK
    # Expected: [{'_type': 'B', 'a': 1, 'b': 2}, {'_type': 'B', 'a': 3, 'b': 4}]
@Tinche
Copy link
Member

Tinche commented Mar 9, 2021

Ah, I see your issue.

Historically, the semantics of unstructuring were inherited from attr.asdict, since cattrs in a way grew out of attrs. The attrs version of asdict uses the runtime class of whatever it's unstructuring, so it obviously can't work with more abstract types (like Optional, Union, MutableSequence).

A neat benefit of this is also that unstructure hooks can be very simple, for example if you want to stringify something while unstructuring, you just register the str function as the unstructure hook, instead of having to use a lambda and swallowing the type param, like you'd need for structuring hooks.

There's also the problem of backwards compatibility to take into consideration.

I agree having a type parameter in there is useful. The initial motivating case for unstructure_as in converter.unstructure was handling unions in a generic way, for example. The problem is this isn't recursive. So in your example, you can override the type of b (so we handle it as mutable sequence, not list, but it doesn't reach deeper. attrs classes carry this information in their type annotation, so there you can use MutableSequence[A] and it will respect the A.

@Dr-ZeeD
Copy link
Author

Dr-ZeeD commented Mar 9, 2021

Thanks for your answer, however if I now introduce

@define
class HasSequenceOfA:
    sequence_of_a: MutableSequence[A]

and do

print(converter.unstructure(HasSequenceOfA(b)))

I still only get

{'sequence_of_a': [{'a': 1, 'b': 2}, {'a': 3, 'b': 4}]}

Am I missing something?

@Tinche
Copy link
Member

Tinche commented Mar 9, 2021

Hm, I was mistaken. Let me see if I can fix this in the next couple of days.

@Tinche
Copy link
Member

Tinche commented Mar 11, 2021

I think I have the solution in https://github.com/Tinche/cattrs/tree/fix-unstuct-iterable-type

@Tinche
Copy link
Member

Tinche commented Mar 12, 2021

This has been merged into master!

@Tinche Tinche closed this as completed Mar 12, 2021
@Dr-ZeeD
Copy link
Author

Dr-ZeeD commented Mar 12, 2021

Awesome, I'll give it a spin over the weekend

@pattonw
Copy link

pattonw commented Mar 12, 2021

Just tested this out. Seems to work on List[Union[A, B]], and Tuple[Union[A,B], ...], but not Optional[Union[A,B]].

@pattonw
Copy link

pattonw commented Mar 12, 2021

This is what I used:

c = cattrs.GenConverter()

@attr.define
class A:
    a: int

@attr.define
class B:
    a: int

@attr.define
class C:
    f: Optional[Union[A, B]]

c.register_unstructure_hook(
    Union[A, B],
    lambda o: {"_type": o.__class__.__name__, **c.unstructure(o)},
)
c.register_structure_hook(
    Union[A, B], lambda o, t: c.structure(o, A if o["_type"] == "A" else B)
)

inst = C(A(1))
unstructured = c.unstructure(inst)
assert unstructured["f"]["_type"] == "A"

assert c.structure(unstructured, C) == inst

@Tinche
Copy link
Member

Tinche commented Mar 12, 2021

Unions are a little special, but they should work. Will take a look.

@Tinche Tinche reopened this Mar 12, 2021
@Tinche
Copy link
Member

Tinche commented Mar 12, 2021

@pattonw Actually I see the issue. Optional[Union[A, B]] reduces to Union[A, B, None], and your hook is for Union[A,B], so it not getting triggered. Confirm for me?

@pattonw
Copy link

pattonw commented Mar 12, 2021

Hmm, this doesn't work either. But this also doesn't seem like an ergonomic way of writing this hook.

c = cattrs.GenConverter()

@attr.define
class A:
    a: int

@attr.define
class B:
    a: int

@attr.define
class C:
    f: Optional[Union[A, B]]

c.register_unstructure_hook(
    Union[A, B, None],
    lambda o: {"_type": o.__class__.__name__, **c.unstructure(o)}
    if o is not None
    else None,
)
c.register_structure_hook(
    Union[A, B, None],
    lambda o, t: c.structure(
        o, eval(o["_type"]) if o is not None else None
    ),
)

inst = C(A(1))
unstructured = c.unstructure(inst)
assert unstructured["f"]["_type"] == "A"

assert c.structure(unstructured, C) == inst

@Tinche
Copy link
Member

Tinche commented Mar 12, 2021

This works:

from typing import Optional, Union

import attr

import cattr

c = cattr.GenConverter()


@attr.define
class A:
    a: int


@attr.define
class B:
    a: int


@attr.define
class C:
    f: Union[A, B, None]


c.register_unstructure_hook(
    Union[A, B, None],
    lambda o: {"_type": o.__class__.__name__, **c.unstructure(o)}
    if o is not None
    else None,
)
c.register_structure_hook(
    Union[A, B, None],
    lambda o, t: c.structure(o, eval(o["_type"]) if o is not None else None),
)

inst = C(A(1))
unstructured = c.unstructure(inst)
assert unstructured["f"]["_type"] == "A"

assert c.structure(unstructured, C) == inst

The reason your example doesn't work is interesting. As I mentioned, the Python interpreter evaluates Optional[Union[A, B]] as typing.Union[__main__.A, __main__.B, NoneType], which should be the same as Union[A,B,None]. However, it looks like there's caching in the interpreter, so this is true:

Union[A, B, None] is Union[A, B, None]

but this isn't:

Union[A, B, None] is Optional[Union[A, B]]

Anyway, I can fix this by using == instead of is in the hook. I can make the change on master after it passes tests.

@Tinche
Copy link
Member

Tinche commented Mar 12, 2021

Note that in 3.10 the Optional and Union types are going to basically deprecated, so you'll be writing A | B | None anyway.

@Tinche
Copy link
Member

Tinche commented Mar 13, 2021

Pushed a change to master that makes Optional[Union[A,B]] hook match Union[A, B, None] hooks.

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

3 participants