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

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

Open
Drino opened this issue Oct 25, 2020 · 5 comments · May be fixed by #1267
Open

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

Drino opened this issue Oct 25, 2020 · 5 comments · May be fixed by #1267
Labels
Thinking Needs more braining.

Comments

@Drino
Copy link
Contributor

Drino commented Oct 25, 2020

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
Copy link
Contributor

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
Copy link
Member

hynek commented Oct 26, 2020

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
Copy link
Member

hynek commented Oct 26, 2020

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
Copy link
Contributor Author

Drino commented Oct 27, 2020

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 :)

@SubeCraft
Copy link

Any update to this issue ?

@hynek hynek linked a pull request Mar 17, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Thinking Needs more braining.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants