python-attrs / attrs

Python Classes Without Boilerplate
https://www.attrs.org/
MIT License
5.23k stars 364 forks source link

Allow three-argument converters (like validators/on_setattr) #709

Closed Drino closed 1 month ago

Drino commented 3 years ago

I'd like to move the discussion from converter decorator PR to this issue.

I think converters are semantically closer to on_setattr and validator than default. E.g. attr.ib(converter=...) allows you to pass a list of callables and pipes them automatically - exactly like attr.ib(validator=...) and attr.ib(on_setattr=...) and there is attr.converters module, like attr.setters and attr.validators.

If we allow passing half-initialized self to converter, why don't allow full-form converters, converter(self, attr, value) to make them the same as on_setattr, but for initialization?

To support one, two and three-argument converters there should be either inspect-magic (in py2 - getargspec, in py3 - signature) or mandatory Converter wrapper for two and three-argument converters.

sscherfke commented 3 years ago

Maybe, converters and validators can (or event should) be merged (similarly to click callbacks)?

def int_validator(self, attrib, value):
    return int(value)  # Validates and converts at the same time :-)

I guess that would be very backwards incompatible, but maybe this can (still) be added to the new attrs API?

hynek commented 3 years ago

To support one, two and three-argument converters there should be either inspect-magic (in py2 - getargspec, in py3 - signature) or mandatory Converter wrapper for two and three-argument converters.

This is something I've been thinking about in the past days. I think a marker-wrapper for three-argument converters would be most solid and in line with our other APIs?

hynek commented 3 years ago

There's also #146 which still bothers me that it's unresolved but to which I still don't have a good answer, 3.5 years later. :(

Drino commented 3 years ago

Thank you for the discussion!

@sscherfke I think this is kind of thing that happened to on_setattr already. The only big difference here is that converters should work with half-initialized self, but @default shows that it can be tackled by users (and #404 shows that some of them are actually craving that possibility in a converter).

Personally I like separate validators, as they are running after all the conversions (hence - guaranteed to work with fully-initialized self!) and imply that the field value doesn't change (unless some reckless guy will do object.__setattr__(self, attr.name, new_value) inside, but it seems like a dirty hack and wouldn't pass any code-review. Or at least I hope so...)

@hynek #146 is pretty informative, thank you for mentioning it! :) I actually missed the stacktrace point when I was thinking about it (and maybe some performance issues, as there is a level of indirection here...). Though, I think that having on_setattr on board actually introduces some common API for a pipeline here.

I do like the marker-wrapper idea (e.g. Converter(fun, takes_self=True, takes_attr=True)), though it may be a bit unexpected for a newcomer that attr.ib(converter=c) expects one-argument function or wrapper, while attr.ib(validator=v, on_seattr=s) expect three-argument function (and no wrappers provided). But - considering possible issues with half-initialized self - it may be a good practice, actually.

UPD: Just saw the comment in the PR. Actually, if converter is kind of shortcut to something like on_init - it sounds like a great way to get away from this unexpected behavior :)

leafayon commented 7 months ago

Any update to this issue ?

germaniuss commented 2 months ago

After #1267 is merged there would be two distinct ways to do almost the exact same thing.

@define
class MyClass:
    x: int = field(converter=Converter(..., takes_self = True, takes_field = True))

and

@define
class MyClass:
    x: int = field(on_setattr=...)

Would it makes sense to add an on_init = True parameter to Converter since they would be very similar and the only difference is that on_setattr does not run on __init__? Taking it one step further on_setattr could be removed entirely from the code except as an alias for Converter for backwards compatibility

germaniuss commented 2 months ago

Now that I take a look at it a converter takes the value first and then self and attribute which I feel is even more confusing since both validator and on_setattr take self and attribute first.

hynek commented 1 month ago

Now that I take a look at it a converter takes the value first and then self and attribute which I feel is even more confusing since both validator and on_setattr take self and attribute first.

Yeah this is not ideal, however it has symmetry with factories which are the other concept that use takes_self. Nowadays, I would approach this whole thing differently, probably only forcing the value as the first argument and detect the other two at runtime and pass them as kwargs, but for now we have to make do with the cruft we have. :|