An initial implementation of unstructuring and structuring enums with complex values#702
Conversation
|
Thanks for working on this, I will review within a couple days when I get some time. Don't worry about Hypothesis, I'm kind of moving away from it, since it bloats the test times and can turn out to be very complex to use properly, so the cost/benefit ratio is usually in favor of just normal unit tests. :) |
Tinche
left a comment
There was a problem hiding this comment.
Sorry for the delay, I'm a little under the weather.
Looks like good work, left some comments.
HISTORY.md
Outdated
| ([#698](https://github.com/python-attrs/cattrs/pull/698)) | ||
| - Apply the attrs converter to the default value before checking if it is equal to the attribute's value, when `omit_if_default` is true and an attrs converter is specified. | ||
| ([#696](https://github.com/python-attrs/cattrs/pull/696)) | ||
| - Unstructure enum values and structure enum values if they have a type hinted `_value_` attribute. |
There was a problem hiding this comment.
I would rephrase this a little bit, maybe something like:
Take account of the optional value type hint when handling enums, if present.
src/cattrs/converters.py
Outdated
|
|
||
| Uses type hints for the "_value_" attribute if they exist to structure | ||
| the enum values before returning the result.""" | ||
| hints = get_type_hints(cl) |
There was a problem hiding this comment.
get_type_hints is probably going to be expensive. Let's try doing a simple check first. Maybe something like:
if "_value_" in cl.__dict__:
val = self.structure(val, get_type_hints(cl)["_value_"])
I want to avoid doing too much extra work in the majority of cases.
There was a problem hiding this comment.
Gocha. Unfortunately _value_ isn't in __dict__. I am seeing it in __annotations__, so we can do that check, but I don't know for sure if that's an implementation detail or a part of the standard. We can pull the type from there too, though. I'll make the change and push it for now.
src/cattrs/converters.py
Outdated
| """Convert an enum to its value.""" | ||
| return obj.value | ||
| """Convert an enum to its unstructured value.""" | ||
| return self._unstructure_func.dispatch(obj.value.__class__)(obj.value) |
There was a problem hiding this comment.
Hm, I'm a little concerned about performance and backwards comp. Can we do a deep unstructure only if the enum has a _value_ attribute, like when structuring?
There was a problem hiding this comment.
Same as above. Change made so it's conditional like the above as well.
docs/defaulthooks.md
Outdated
| 'siamese' | ||
| ``` | ||
|
|
||
| Enum structuring and unstructuring even works for complex values, like tuples, but if you have anything but simple literal types in those tuples (`str`, `bool`, `int`, `float`) you should consider defining the Enum value's type via the `_value_` attribute's type hint so that cattrs can properly structure it. |
There was a problem hiding this comment.
Let's link https://typing.python.org/en/latest/spec/enums.html#member-values in here somewhere.
No worries on the delay. Comments should be addressed in the additional commits. |
|
LGTM, thanks. I might work on this a little more before releasing the next version, to see if I can convert this code into a hook factory for more speed. I also might not, time-depending. |
Fixes #699. The values of an Enum are unstructured and structured. Structuring is only done when the Enum class has a type defined for the
_value_attribute as per the Python typing standard.I've added two very simple tests which show that this works. I'm attempting to add tests which use hypothesis like the rest but the magic chains of incantations in
tests/typed.pyare throwing me a bit and it's taking a while to understand what they're doing and how to use it to generate random dynamic Enum classes. Any pointers there would be appreciated.