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

[RFC] __setattr__ hooks #645

Closed
hynek opened this issue May 14, 2020 · 6 comments · Fixed by #660
Closed

[RFC] __setattr__ hooks #645

hynek opened this issue May 14, 2020 · 6 comments · Fixed by #660
Labels
Milestone

Comments

@hynek
Copy link
Member

hynek commented May 14, 2020

There's two things that people keep asking for:

  1. validation on setting attributes
  2. freezing single attributes

Those two features have something in common: they require attrs to write a __setattr__ method.

I actually had 1 done when I implemented validators but I took it out again, because I didn't want to tamper with __setattr__ too. But it totally makes sense to expect that validators run there too.

Now that argument has gone away thanks to frozen classes and attrs is in the __setattr__ business. So it feels like the right thing to do, to implement it and make it default for Operation import attrs (I hope this is legit the last part of the puzzle).

To allow for 2 too, I would suggest to add a hook called on_setattr (better names welcome) that takes a callable that is called with the instance, the attribute definition, and the new value.

To solve 2, the implementation would look like

def frozen(_, __, ___):
    raise FrozenInstanceError

Open questions:

  • what to do about on_setattr attributes in a frozen class (incl inheritance)
  • what about converters? Maybe it should take a list/and like validator/converter do? They would need to work as a chain, returning values for the next one.
@Tinche
Copy link
Member

Tinche commented May 17, 2020

I agree not doing validation on attribute set but doing them in __init__ is probably not reasonable and should be changed.

As for the implementation, it'd be ideal if we generated a smart __setattr__ like we generate a smart __init__. If I were doing it I'd write a function basically containing a chain of if name == {attr0.name} elif name == {attr1.name} and compile it; it's gnarly but probably the fastest approach (the things we do for speed :).

Not really sure what you mean with the hook proposal. Don't we have basically two hook frameworks already (validators and converters)? Both of them seem inadequate for frozen attributes though, because they run on __init__ too. Does it make sense to flesh this out more thoroughly?

@Tinche
Copy link
Member

Tinche commented May 17, 2020

I mean, we could flesh out validators so they would know if they are being run in the __init__ context or the __setattr__ context. Then you could implement frozen attributes using a validator.

If I was designing this system right now, I'd probably want to use a middleware pattern (kind of like aiohttp's middleware, where it's a pipeline of functions).

@bitbucketboss
Copy link

(1) When I first started using Attrs I found the lack of a validate-on-set option a bit strange but over time have become used to the validate-after-init approach ultimately still stopping execution when validators fail. So I, personally, dont have a strong preference.

(2) This is a feature I would LOVE to have. To date I've been using my own kludge to get this more or less working but I'd much rather have this implemeted professionally.

Would a class with all attributes frozen singly be functionally equivalent to a frozen class? Presumably not because you can add attributes to a nonfrozen class? What I would really like to have is the ability to have a class functionally frozen with only attributes marked as not-frozen remaining mutable.

PS: Thank you for this amazing package. Using Attrs has resulted in a step change in the quality and productivty of my Python coding.

@hynek
Copy link
Member Author

hynek commented Jun 7, 2020

@Tinche the question really is how to expose the functionality without painting ourselves into a corner. Right now I can think of three reasons the user might want to tinker with __setattr__:

  • validation
  • conversion
  • freezing

I guess that could be achieved using frozen: bool, validate_on_set: bool, convert_on_set: bool with some thing mutually excluding itself. I suspect tho that it might paint us into a corner (or force us adding dozens more of such arguments) so my suggestion is to approach from on_setattr=pipe(convert, validate) (that’s how they run in __init__ too) and allow people to do whatever they want on on_setattr. Hopefully coming up with creative ways we didn’t think of. Freezing would become on_setattr=explode.


As for the implementation itself I suspect there’s a cut off point where a dict is faster than a long if-then-else but that remains to be benchmarked.

@hynek
Copy link
Member Author

hynek commented Jun 7, 2020

Ah #622 is an example.

@hynek
Copy link
Member Author

hynek commented Jul 10, 2020

Please y'all have a look at #660.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants