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

Using class methods strategy for structuring Enums fails without clear info #479

Open
EFord36 opened this issue Jan 3, 2024 · 4 comments

Comments

@EFord36
Copy link
Contributor

EFord36 commented Jan 3, 2024

  • cattrs version: 23.2.3
  • Python version: Python 3.9.16
  • Operating System: macOS Ventura 13.5.2 (22G91)

Description

I want to ultimately serialize an Enum (anywhere it appears in dataclasses) using its name, rather than its value (as the value is an integer that isn't very human interpretable, whereas the name is).

It looks like for some of my other classes, the use_class_methods strategy will be very convenient, so I thought I would use that here. I know there are other approach I could and probably should take, but I was fairly surprised by the resulting behaviour so thought it was worth an issue.

What I Did

If I run this python script:

from enum import Enum

from cattrs import Converter
from cattrs.strategies import use_class_methods

class MyEnum(Enum):
    SomeName = 5
    AnotherName = 12
    @classmethod
    def _structure(cls, data: str) -> "MyEnum":
        return cls[data]
    def _unstructure(self) -> str:
        return self.name

converter = Converter()
use_class_methods(converter, "_structure", "_unstructure")

print(converter.unstructure(MyEnum["SomeName"]))
print(converter.structure(5, MyEnum))
print(converter.structure("SomeName", MyEnum))

The output show that the unstructuring behaviour is what I want (using the name), but structuring still expects a value, and doesn't work with a name:

SomeName
MyEnum.SomeName
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/ktdb353/workplace/kazu/.venv/lib/python3.9/site-packages/cattrs/converters.py", line 332, in structure
    return self._structure_func.dispatch(cl)(obj, cl)
  File "/Users/ktdb353/workplace/kazu/.venv/lib/python3.9/site-packages/cattrs/converters.py", line 430, in _structure_call
    return cl(obj)
  File "/Users/ktdb353/.pyenv/versions/3.9.16/lib/python3.9/enum.py", line 384, in __call__
    return cls.__new__(cls, value)
  File "/Users/ktdb353/.pyenv/versions/3.9.16/lib/python3.9/enum.py", line 702, in __new__
    raise ve_exc
ValueError: 'SomeName' is not a valid MyEnum

This now means you can't 'round-trip' with an unstructure and then a structure, or you will get an error, which feels quite unexpected.

It appears to me that this is because _structure_call is registered to handle Enum within the _single_dispatch for structure, which is tried before the function_dispatch that the use_class_methods function is using, so MyEnum._structure is never called.

By comparison, within unstructured, there's nothing for Enums until you get to the function_dispatch, so it does use the method from the class.

Is it worth doing something like when registering the class method functions, checking if there's a single or direct dispatch that will handle the class, and therefore if the method will never be used, and if so to log a warning or raise an error? Equally, the risk is something could be added later to the single or direct dispatch that still causes it to never be called.

@EFord36
Copy link
Contributor Author

EFord36 commented Jan 3, 2024

I also appreciate a valid answer may be "don't try and override structuring of such primitive types, and change your Enum to have values that are a tuple of the human readable name and the integer", but I personally want to avoid this since:

  1. It would be a backwards-incompatible change to my library
  2. I'm using an IntEnum which means I can compare values of the Enum directly, rather than having to look inside the enum and get the integer to do this - this is a very convenient feature.

Maybe it would just be enough to be explicit in this section of the docs that custom structuring of Enums isn't supported? (And maybe the same in other relevant section(s) for other primitives).

@Tinche
Copy link
Member

Tinche commented Jan 3, 2024

The use_class_methods strategy was originally meant to be used with on attrs classes and dataclasses, but I see no reason why enums should be excluded. I'll mark this issue as an enhancement request.

I also appreciate a valid answer may be "don't try and override structuring of such primitive types, and change your Enum to have values that are a tuple of the human readable name and the integer"

The point of cattrs is customizability so I wouldn't consider this a valid answer ;)

You've identified the issue correctly. I think a good long-term fix would be to use predicate hooks for enums too.

For the time being, you can fix your issue manually. Maybe something like this?

from enum import Enum

from cattrs import Converter
from cattrs.strategies import use_class_methods


class MyEnum(Enum):
    SomeName = 5
    AnotherName = 12

    @classmethod
    def _structure(cls, data: str) -> "MyEnum":
        return cls[data]

    def _unstructure(self) -> str:
        return self.name


converter = Converter()
use_class_methods(converter, unstructure_method_name="_unstructure")

converter.register_structure_hook(MyEnum, lambda v, _: MyEnum._structure(v))

print(converter.unstructure(MyEnum.SomeName))
# print(converter.structure(5, MyEnum))
print(converter.structure("SomeName", MyEnum))

@Tinche
Copy link
Member

Tinche commented Jan 3, 2024

Maybe it would just be enough to be explicit in this section of the docs that custom structuring of Enums isn't supported?

Custom structuring of enums should be very supported ;)

@EFord36
Copy link
Contributor Author

EFord36 commented Jan 3, 2024

Thank you so much for the very quick response, willingness to consider the enhancement and for providing the 'manual' code to get it working!

Really loving cattrs so far (apart from this minor issue), so once I've integrated it into my code, maybe I can try a PR to support this to give just a tiny bit back!

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

No branches or pull requests

2 participants