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

Allow structuring union types of primitives that don't need any conversion #278

Closed
phiresky opened this issue Jun 20, 2022 · 14 comments · Fixed by #403
Closed

Allow structuring union types of primitives that don't need any conversion #278

phiresky opened this issue Jun 20, 2022 · 14 comments · Fixed by #403
Milestone

Comments

@phiresky
Copy link

  • cattrs version: current master
  • Python version: 3.10
  • Operating System: Linux
  • Let me know when you get annoyed by my issues ;)

Description

cattrs supports passing through primitives as well as the type Literal["foo", "bar"]

I have three examples of types that cattrs currently does not support:

  • Union[int, typing.Literal["foo"]]
  • Union[Literal["a", "b"], Literal["c", "d"]]
  • T2 = NewType("T2", str) then Literal["train", "test"] | T2)

I think cattrs should pass through all forms of combinations of primitives, literals, and primitive newtypes.

Here's an implementation that works for me:

def is_primitive_or_primitive_union(t: Any) -> bool:
    """returns true if given type is something that doesn't need ser/deser"""
    if t in (str, bytes, int, float, bool):
        return True
    origin = typing.get_origin(t)
    if origin is typing.Literal:
        return True
    if (basetype := cattrs._compat.get_newtype_base(t)) is not None:
        return is_primitive_or_primitive_union(basetype)
    if origin in (types.UnionType, typing.Union):
        return all(is_primitive_or_primitive_union(ty) for ty in typing.get_args(t))



[...]

 _converter.register_structure_hook_func(
    is_primitive_or_primitive_union, lambda v, ty: v
)


if __name__ == "__main__":
    T2 = NewType("T2", str)
    print(is_primitive_or_primitive_union(Union[int, typing.Literal["silu"]]))
    print(is_primitive_or_primitive_union(Union[Literal["a", "b"], Literal["c", "d"]]))
    print(is_primitive_or_primitive_union(Literal["train", "val", "test"] | T2))
@Tinche
Copy link
Member

Tinche commented Jun 26, 2022

Yep, I agree we should do this. Should the list of primitive types be on the preconf converters though? Feel like different serialization formats might support different primitives.

@phiresky
Copy link
Author

That's true. I was only really thinking of the basic things that JSON supports. Probably pretty much every serialization format supports those. Maybe it could be a flag passthrough_types that by default includes something like float,int,str,bool,None.

@phiresky
Copy link
Author

But aren't you already making that assumption? Since Literal[1, True, "foo"] already works

@jonasrauber
Copy link

jonasrauber commented Jul 28, 2022

I think supporting Unions of primitive types out of the box would definitely be great. But I don't think the above behaves as expected because it also passes through types that are not part of the Union.

Here is a minimal example:

>>> cattrs.structure("foo", Union[int, float])
"foo"

To me at least, that would be an unexpected result.
I rather would want the above case to fail like cattrs.structure("foo", int) and cattrs.structure("foo", float) do.

@phiresky
Copy link
Author

Yeah, that's true. I didn't really care about that and it's still working ok for me (the validation for me comes from mypy), but checking the type at runtime should probably be added.

@jonasrauber
Copy link

Out of curiosity: how does mypy help you here?

@phiresky
Copy link
Author

For me all stuff that's loaded via cattrs is stuff that was at some point earlier serialized with cattrs. So before being serialized it was also type checked by mypy. This doesn't apply if you use cattrs to deserialize things that come from outside your code base though.

@aquamatthias
Copy link

The examples shown here use literals. The case I have is even simpler:

@define
class KubernetesServicePort:
    target_port: Optional[Union[str, int]] = field(default=None)

When I try to structure json back into a service port, I get this error:

| cattrs.errors.ClassValidationError: While structuring KubernetesServicePort (1 sub-exception)
 +-+---------------- 1 ----------------
   | Traceback (most recent call last):
   |   File "<cattrs generated structure resoto_plugin_k8s.resources.KubernetesServicePort>", line 36, in structure_KubernetesServicePort
   |     res['target_port'] = __c_structure_target_port(o['target_port'], __c_type_target_port)
   |   File "/xxx/site-packages/cattrs/converters.py", line 346, in _structure_error
   |     raise StructureHandlerNotFoundError(msg, type_=cl)
   | cattrs.errors.StructureHandlerNotFoundError: Unsupported type: typing.Union[str, int, NoneType]. Register a structure hook for it.

I can define a structure handler like this:

converter.register_structure_hook(Union[str, int, NoneType], lambda obj, typ: obj)

which "solves" the problem - but I think this is rather pointless.
Is there any better way of solving this?

@Tinche
Copy link
Member

Tinche commented Nov 4, 2022

I'll keep this in mind for the next version. Definitely sounds useful to have.

@Tinche Tinche added this to the 22.3 milestone Nov 4, 2022
@Tinche
Copy link
Member

Tinche commented Nov 4, 2022

I can put something together in the mean time and paste it here. It's solvable currently, just requires some expertise.

@aquamatthias
Copy link

That would be very much appreciated. I have all sorts of unions of primitive types. It looks like I currently have to define it explicitly for every combination..

@phiresky
Copy link
Author

phiresky commented Nov 4, 2022

Look at the code in my original issue description. It solves this problem for all combinations of unions of literals, primitives, newtypes with a single hook.

@aquamatthias
Copy link

aquamatthias commented Nov 4, 2022

@phiresky Ah should have read more carefully - thanks. The provided code does not handle the None case, which is included for all optional fields.

I added the None case to the list of primitives and this works.

def is_primitive_or_primitive_union(t: Any) -> bool:
    if t in (str, bytes, int, float, bool, NoneType):
        return True
    origin = get_origin(t)
    if origin is Literal:
        return True
    if (basetype := cattrs._compat.get_newtype_base(t)) is not None:
        return is_primitive_or_primitive_union(basetype)
    if origin in (UnionType, Union):
        return all(is_primitive_or_primitive_union(ty) for ty in get_args(t))
    return False

@Tinche Tinche modified the milestones: 23.1, 23.2 May 23, 2023
@Tinche Tinche linked a pull request Aug 17, 2023 that will close this issue
@Tinche
Copy link
Member

Tinche commented Aug 17, 2023

The support for this is very close to being merged!

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

Successfully merging a pull request may close this issue.

4 participants