feat: strategy for using class methods

[pohlt]

Here's a new PR for #394.

• Code
• Tests
• Docs

[Tinche]

Pretty cool, thanks! I'll take a look in the next few days.

[pohlt]

My local black is configured for a line length of 100 characters.
Reformatted with 88 and updated the branch.

Any reason why you don't use pre-commit to check potential commits locally?

[Tinche]

I'm not a big fan of precommit, the dummy configuration here is I think inherited from the parent organization. You can use make lint or tox -e lint to run the lints ;)

How would you feel if we made it so that if the given function takes two arguments, we also make the strategy pass in the converter too? I'm thinking it might be convenient and easy to support.

@define
class MyClass:
    a: int
    b: Nested

    @classmethod
    def _structure(cls, data: dict, converter):
        return cls(data["b"] + 1, converter.structure(data['b'], Nested))

    def _unstructure(self):
        return {"c": self.a - 1}  # unstructuring as "c", not "a"

[pohlt]

Makes a lot of sense. I didn't think of nested structures.
Maybe I will find some time this week to add it.

[pohlt]

Just to make sure we are one the same page: That would mean changing stuff in dispatch.py, right?

How would you handle the distinction between functions accepting and not accepting a converter argument? inspect.signature, try ... except, ...?

[Tinche]

Yeah, I figured we'd use inspect.signature. I don't think it needs changes other than the strategy, though. Here's my suggestion (for structure at least):

def use_class_methods(
    converter: BaseConverter,
    structure_method_name: Optional[str] = None,
    unstructure_method_name: Optional[str] = None,
) -> None:
    if structure_method_name:

        def make_class_method_structure(cl: Type) -> Callable:
            fn = getattr(cl, structure_method_name)
            sig = signature(fn)
            if len(sig.parameters) == 1:
                return lambda v, _: fn(v)
            if len(sig.parameters) == 2:
                return lambda v, _: fn(v, converter)
            raise TypeError("Provide a class method with one or two arguments.")

        converter.register_structure_hook_factory(
            lambda t: hasattr(t, structure_method_name), make_class_method_structure
        )

    if unstructure_method_name:
        converter.register_unstructure_hook_func(
            lambda t: hasattr(t, unstructure_method_name),
            lambda v: getattr(v, unstructure_method_name)(),
        )

I've switched to register_structure_hook_factory which is one level up, instead of a generic structure function we use a factory of structure functions which is very handy here. I'll leave it to you to adjust the unstructure path accordingly if this works for you ;)

[pohlt]

How about this? No factory, but a little bit shorter:

def use_class_methods(
    converter: BaseConverter,
    structure_method_name: Optional[str] = None,
    unstructure_method_name: Optional[str] = None,
) -> None:

    def call_wrapper(n, f):
        n_parameters = len(signature(f).parameters)
        if n_parameters == n:
            return f
        if n_parameters == n + 1:
            return lambda *v: f(*v, converter)
        raise TypeError("Provide a class method with one or two arguments.")

    if structure_method_name:
        converter.register_structure_hook_func(
            lambda t: hasattr(t, structure_method_name),
            lambda v, t: call_wrapper(1, getattr(t, structure_method_name))(v),
        )

    if unstructure_method_name:
        converter.register_unstructure_hook_func(
            lambda t: hasattr(t, unstructure_method_name),
            lambda v: call_wrapper(0, getattr(v, unstructure_method_name))(),
        )

P.S.: The error text has to be adapted for the unstructure case.

[Tinche]

That'll work, but the issue there is that it calls signature on each un/structure and the actual signature of those methods isn't going to change, so it's inefficient. And signature is a relatively slow function.

[Tinche]

Sorry, misclicked Close

[pohlt]

You're right. I used your approach and added tests and docs. See #405

[Tinche]

Looks great! I might tinker with the docs after you fix the tests, but solid work.

[pohlt]

When writing the _structure method I realized that I have to do the resolution of the union type.
Would it be possible to do a structuring into a class attribute?

  @define
    class Nested:
        a: Union["Nested", None]
        c: int

        @classmethod
        def _structure(cls, data, conv):
            b = data["b"]
            return cls(None if b is None else conv.structure(b, cls), data["c"])

            # Would something like this be possible?
            return cls(conv.structure_partial(b, Nested.a), data["c"])  # or structure_partial(b, Nested, "a")

[Tinche]

You can do that today by just using structure:

from typing import Union

from attrs import define, fields, resolve_types

from cattrs import Converter
from cattrs.strategies import use_class_methods


@define
class Nested:
    a: "Union[Nested, None]"
    c: int

    @classmethod
    def _structure(cls, data, conv):
        b = data["b"]

        # Would something like this be possible?
        return cls(conv.structure(b, fields(Nested).a.type), data["c"])


resolve_types(Nested)

c = Converter()
use_class_methods(c, "_structure")
print(c.structure({"b": {"b": None, "c": 1}, "c": 1}, Nested))

The biggest issue is how Python handles stringified type annotations, which are necessary for self-referencing types, hence the call to resolve_types. This should be improved in Python 3.13 ;)

[pohlt]

Awesome! I changed the test to use your version.

And I hopefully fixed all remaining issues with Python < 3.10.

[pohlt]

I don't understand the CI issues. Judging by the logs, the installation process just stalls. Just a hiccup on the CI runner?

[Tinche]

They look fine to me. On this line: https://github.com/python-attrs/cattrs/pull/405/files#diff-40771e4a7b976ef4fbc939a5524111d62406bf9162a342dff6ac335c33f33948R38 you need to use typing.Type instead of type for Python 3.7 and 3.8, since on those versions type[] doesn't work.

[pohlt]

Strange. For me, the logs just stopped. Anyway, type[] is fixed now. Thanks for the hint.

[Tinche]

Looks like https://github.com/python-attrs/cattrs/pull/405/files#diff-40771e4a7b976ef4fbc939a5524111d62406bf9162a342dff6ac335c33f33948R13 needs to be switched over to the old Union syntax too, for older Pythons.

[Tinche]

Cool, thanks!

[pohlt]

Glad I could contribute something.
Thanks for the energy and thought you put into (c)attrs. Highly appreciated!