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

Add init_validation flag #233

Closed
wants to merge 3 commits into from
Closed

Conversation

matthayes
Copy link

This adds an init_validation flag to attr.s that controls whether validators are run at the end of the __init__() method. This allows one to choose when validation should be run, which can be useful in some situations.

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Updated documentation for changed code.
  • Documentation in .rst files is written using semantic newlines.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) are documented in CHANGELOG.rst.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

init_validation controls whether validation runs during __init__.  By default it is True, which is the current behavior.  If it is set to False then validators will not be run.
@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

Merging #233 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #233   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         603    603           
  Branches      132    132           
=====================================
  Hits          603    603
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57f3d38...e935b95. Read the comment docs.

@matthayes
Copy link
Author

By the way, I can add more documentation if that is helpful. Initially I wanted to confirm that this is an okay change to make before investing time in that.

@hynek
Copy link
Member

hynek commented Aug 18, 2017

I don’t know if it’s me but it seems really weird to me to define validators on a class and tell that class to not run the at the same time?

The reason why the setting is currently out of the class is that you may want to disable them under certain conditions that are none of the class’ business…

@matthayes
Copy link
Author

I want explicit control over when the validators run. Validation can sometimes be expensive depending on the class. Because the validators run automatically in __init__ I had to write a separate validate method with all the checks that I would much rather declare in the attr.ib() definitions. Validation doesn't run automatically when setting attributes so it doesn't seem that weird to me to choose not to run it automatically in __init__ either.

There are two approaches I could think of: 1) Add a init_validation flag to attr.s so that validation can be disabled always during __init__, or 2) Add an optional _validate flag to __init__ so it can be disabled on a case-by-case basis. I don't have a strong preference between the two but ended up going with the first. I don't really like the global flag.

@hynek
Copy link
Member

hynek commented Aug 18, 2017

I see – I have never considered validators something that has any value outside __init__. If you wanna go down that road, wouldn’t it be more consequent to have a run_validators=NEVER|ALWAYS|INIT|SETATTR?

@wsanchez
Copy link

wsanchez commented Aug 18, 2017

I think there's a good argument for running validation when setting an attribute.

In both the init and setter case, though, it seems like skipping validation should be a thing you decide at init/set time, rather than at class definition time, though… no?

Deciding this on this class-level still a global setting, though a narrower one.

@matthayes
Copy link
Author

@hynek Enumerating the validation options like that makes sense to me. Seems these would be defined as:

  • NEVER: never run validators automatically
  • ALWAYS: run validators automatically in __init__ and when setting attributes
  • INIT: run validators automatically in __init__ only (current behavior)
  • SETATTR: run validators automatically when setting attributes only

@wsanchez I agree there's a good argument for at least making it configurable (as above) whether validation should be run automatically when setting an attribute. Regarding what you said about deciding at init/set time, I think it's reasonable to have an option to configure at the class level. If you have a class where don't want validation to run most of the time, then it's easier to be able to set it at the class level. This is how I coded my class with the validate method, and it would be nice to rely on attrs instead.

Having the ability to decide at init/set time could be useful too though in addition to having the class level configurability. I can think of a couple potentially useful methods for controlling attribute validation.

frac = Fraction(num=1, den=2)

frac.den = 0    # raises exception
frac = Fraction(num=1, den=2)

with frac.no_validation():
  frac.den = 0    # does not raise exception
frac = Fraction(num=1, den=2)

# validates on exit and does not raise exception
with frac.defer_validation():
  frac.den = 0
  frac.den = 1
frac = Fraction(num=1, den=2)

# validates on exit and raises exception
with frac.defer_validation():
  frac.den = 0
  frac.den = 1
  frac.den = 0

However with no_validation and defer_validation methods like this then ALWAYS may be misleading because it doesn't always run. Instead it runs by default unless overridden.

I would prefer a with context pattern like this over the global setting that exists now because it only applies to the class. With the global setting my worries are 1) you need write a custom context manager anyways to ensure that validation is reenabled (assuming you don't want it permanently disabled), and 2) inadvertently disabling validation in a 3rd party library I'm using that happens to be using attrs too.

@Tinche
Copy link
Member

Tinche commented Aug 18, 2017

Here's my thought process on this issue.

At first I was confused since what's the point of defining validators but never running them, but then I got reminded by the PR comment that we can run validation manually. This feature slipped my mind completely. :)

Then I realized I have a similar situation at work; I load a bunch of data from an Excel file into my attrs classes, validate everything, and serialize the classes into Redis. Once the data is in Redis, it can be loaded without validation; validation is an unnecessary overhead at that point. So this interests me on a practical level.

(Site note: this problem is solvable currently by sticking your validators somewhere else [like the metadata] and writing your own version of attr.validate, which is simple. So I don't think we need to rush to a blessed solution too quickly.)

My gut reaction towards this proposed solution was negative, however. A more elegant solution is probably possible and we should brainstorm first.

The question of running validators on set is interesting. This requires special descriptors; finally removing deprecated C.a attributes for dict classes and Cython for slot classes. These are planned features but not here yet. I think once the infrastructure is in place we should support setter validation for sure, and probably turn in on by default. This is not a trivial feature, for example what about converters, do they get called on set? I think this is a feature unto itself and should go through the normal design/implementation pipeline.

What I think would be really interesting is exposing some of the attrs machinery for generating __init__ to users in some way. Our init generating code is quite sophisticated but all of it is internal. The amount of code we have just for init speaks to the importance of it as well.

This is one of the ways we could support generating static factory methods. The normal __init__ could do validation, but you could have a special generated static factory method that doesn't. (Implementation-wise it'd probably be easier to have the default init not do validation, and the static factory method to just validate additionally, but UX-wise I think the default should be the conservative/slow/safe one.)

We already have another issue open (#187) dealing with reworking the global validation switch; again a testament to the complexity of our init handling code. This issue could be resolved by exposing some of our __init__ machinery too.

@hynek
Copy link
Member

hynek commented Aug 19, 2017

Ehm __setattr__. :) I had it working before, but I ripped it out because I didn't want to fuzz with __setattr__. That ship has sailed now tho.

@matthayes
Copy link
Author

I load a bunch of data from an Excel file into my attrs classes, validate everything, and serialize the classes into Redis. Once the data is in Redis, it can be loaded without validation; validation is an unnecessary overhead at that point.

This is the same situation I am in and what inspired this PR. I have already validated the data and serialized into a DB, so any further validation after is unnecessary and wasteful.

What I think would be really interesting is exposing some of the attrs machinery for generating init to users in some way. Our init generating code is quite sophisticated but all of it is internal. The amount of code we have just for init speaks to the importance of it as well.

Yea what I basically want is a way to customize how __init__ is generated so that I don't have to run the validators if I don't want to. A static method that is more customized as you suggest would work too as this achieves the same end.

@hynek
Copy link
Member

hynek commented Aug 30, 2017

So just to be clear before I leave on my vacation: I’m not opposed to this feature, but I’m opposed to init_validation because it doesn't give us room to grow aside from adding a gazillion more options. We need to find a more general way to control validators if that makes sense to y'all?

@ZivotJeKrasny
Copy link

Hi,
let me ask you for what is the status of this PR?
I have similar issue. It will be very handy to disable validation when loading items from DB. The main reason is that some of my validation are relatively cpu intensive.

@matthayes
Copy link
Author

Hi,
let me ask you for what is the status of this PR?
I have similar issue. It will be very handy to disable validation when loading items from DB. The main reason is that some of my validation are relatively cpu intensive.

I haven't had time to continue working on this. If you are interested you could pick it up.

@Skylion007
Copy link

Any word on this? I have a use case that could benefit from this PR.

@Skylion007
Copy link

Actually, #750 makes this trivial to implement.

Base automatically changed from master to main February 25, 2021 07:39
@hynek
Copy link
Member

hynek commented Nov 21, 2021

I think given the age and overall comment, I'm gonna close this. I personally don't think it makes sense to make our validators "manual". I 100% support the use-case (as does Tin), I just don't think of attrs's validators worth it given much more superior solutions out there like cattrs.

It's always difficult to delineate when to use ours vs when to use a more powerful framework and that's why I've let this PR linger this long so gracelessly. I'm gonna stick a pole in the ground and say from our side, our validators are for when you want to run them always.

For use-cases like tests etc, there's always the way to disable them temporarily (e.g. using the brand new #859).

Sorry everyone who's disappointed!

@hynek hynek closed this Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants