-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Add a helper API for assoc + validate #116
Comments
Huh! I kind of would’ve expected that I feel like it should!? I don’t even care as much about possible breakage as about the implicit performance regression. But |
Agreed. I was surprised that assoc doesn't create a new object using My gut feeling is it's ok to expect assoc to require an attrs initializer, but I've used Clojure so that maybe colors it. Maybe introduce this requirement and change the implementation to not use copy.copy? I can provide some benchmarks in the evening. |
A small addendum: I'm not sure about So there might be a performance win here? |
Show us the numbers sir! |
I'd be happy to have |
I'm linking a module containing three classes of increasing complexity, and a new implementation of https://gist.github.com/Tinche/372796f6e0e899ba099810981d5da541 Note that for old
In more complex classes validators and converters dominate, I think. The
this is what would happen if you tried feeding a non-existent field into an ordinary Python class initializer. To me this is more logical. I've implemented the old way to not break compatibility and pass the tests though. Here are the actual commands if you want to reproduce:
If this looks OK, a pull request can easily be made. |
Such vroom vroom, much fast, very nice! There’s one thing though: does this work with aliased attributes? (i.e. |
Hm. What's the desired behavior?
I guess it comes down to whether you consider I'll take a look at what it takes and what the numbers are. |
good question :| it’s kind of both |
I’ve meditated over it and it’s totally a setter but we should support both which IMHO shouldn’t be a big deal. We’d have to look twice anyways. I just hope it’s possible to implement without measurable penalties to normal cases. But as I see it, it’s rather faster? |
I have a version that works with private attributes, just haven't gotten around to submitting it yet. Uh, support both ways? I think it can be done with a performance penalty only for private attributes. Semi-relatedly, I think it'd be useful to have a comprehensive benchmark suite for attrs. I don't exactly know what this would look like. |
Agreed and I have no idea too. :-| Travis is not deterministic enough so we could just write a benchmark that can be ran locally to get some numbers but I don’t think we can do something automatic. |
This has been fixed by evolve which happens to be faster and all-around prettier! |
evolve: #135 |
It's common to want to do:
It would be nice if the easy, obvious way of making a copy of an object with some changes had the validation built in (instead of the easy, obvious way skipping validation).
The text was updated successfully, but these errors were encountered: