Codecov Report

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

@@          Coverage Diff          @@
##           master   #233   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         603    603           
  Branches      132    132           
=====================================
  Hits          603    603

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.

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.

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…

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.

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?

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.

@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.

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.

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.

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.

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?

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.

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.