Feature request: add a new disambiguation strategy

matmel commented 2 years ago

Description

As mentioned in the tips for handling unions in the documentation, one easy way to resolve the union is to incorporate the type name as an additional key during the destructuring / structuring process. I would like to add a disambiguation function of this nature because the need is even more pressing when you deal with subclasses.

At the moment the union resolving strategy is based on unique fields of attrs classes that do not have defaults. It is actually quite easy in the subclassing union type context to get into the situation where one (or all) of the classes participating in the union have unique fields but with defaults. I think that the type name strategy is a good alternative.

I am wondering the following:

Where does the no default policy comes from? Is it due to the potential of _cattrs_omit_if_default=True argument of make_dict_unstructure_fn?
If and when PR #312 is accepted, I would like to send another PR with this type name key disambiguation strategy presented as an option before cutting a new release due to the predominance of the problem with subclasses. I already have a proof of concept in the type_disambiguation branch of my fork but I am waiting to have the subclasses API settled (re: customize method of Converter) before sending this new PR.

@Tinche thoughts?

Tinche commented 2 years ago

I agree we should add a new disambiguation strategy based on tagged unions (the nomenclature is a little fuzzy here since there are a lot of terms being used in slightly different contexts). Like you said, the idea would be to inject a piece of data into the payload during unstructuring, and to use that piece during structuring. I think we should be very flexible in what that piece of data is, probably allowing the user to pass in a callable or a dictionary when creating the strategy, and a default class when that piece of data is not present. Could be the class name, the fully qualified class name, maybe a classvar on each of the classes, maybe a hardcoded mapping. We also need to be able to customize the key under which this piece of data is being stored. I think this would be super useful to have.

The current default strategy is very simple, since cattrs aims for simple defaults. We also can't change the default strategy for backwards compatibility reasons I think. But new strategies should be easily pluggable by the user.

The default strategy only works with fields with no defaults because it's not safe to do otherwise in a general sense.

Say you have two classes:

@define
class A:
    a: int = 0

@define
class B:
    b: int = 0

And you get a payload of {}. Both A and B can successfully be structured from this payload ;)

Keep in mind cattrs doesn't assume it's being used at both ends; we need to be able to structure unions from third parties, and third parties often omit optional keys from payloads.

We can get the tagged union support in before the next release, no worries.

matmel commented 2 years ago

Thanks for taking the time to make a detailed answer.

I think we should be very flexible in what that piece of data is

Agreed. I was starting to implement a subset of the features you mentioned but I thought I went a little overboard. I reduced the scope in my initial implementation by just giving the choice of the key name to add in the payload. One difficulty I had is how to transfer all those options down and up the stack to reach the disambiguation generator function. I am eager to see your customize method, it sounds promising with respect to those kind of issues.

We also can't change the default strategy for backwards compatibility reasons I think.

Agreed and this is what I have implemented. The alternate strategy is opt-in and not the default.

we need to be able to structure unions from third parties, and third parties often omit optional keys from payloads.

Thanks that is some context I was missing. It makes sense.

Tinche commented 2 years ago

Hi,

I've got a PR with the tagged union strategy here: https://github.com/python-attrs/cattrs/pull/318

There's a way to apply this to the include_subclasses strategy easily, I think.

Let me know what you think.

matmel commented 2 years ago

I did not test on my use case yet, but the documentation and the features that you propose are definitely well in line with what I would need.

Tinche commented 1 year ago

I think this is complete?

python-attrs / cattrs

Feature request: add a new disambiguation strategy #317

Description