Make optional support lists of validators

[hynek]

Fixes #161

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.

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #186   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         554    560    +6     
  Branches      122    124    +2     
=====================================
+ Hits          554    560    +6

Impacted Files	Coverage Δ
src/attr/validators.py	100% <100%> (ø)	⬆️
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 fdfd51e...772174d. Read the comment docs.

[Tinche]

So a small problem with this is that it makes the existing use case, with optional+just one subvalidator ~10% slower. I tried this approach:

def optional(validator):
    """
    A validator that makes an attribute optional.  An optional attribute is one
    which can be set to ``None`` in addition to satisfying the requirements of
    the sub-validator.

    :param validator: A validator (or a list of validators) that is used for
        non-``None`` values.
    :type validator: callable or :class:`list` of callables.

    .. versionchanged:: 17.1.0 *validator* can be a list of validators.
    """
    if isinstance(validator, list):
        def chain_validator(inst, attr, val):
            for v in validator:
                v(inst, attr, val)
        return _OptionalValidator(chain_validator)
    return _OptionalValidator(validator)

This works (no overhead on one subvalidator) but this is ~10% slower for a list of subvalidators (and it doesn't have a nice repr). So why not just add an _OptionalValidators class in addition to the existing _OptionalValidator so we can have our cake and eat it too.

Do we want to offer easy validator chaining as an API? Something like

from attr.validators import and_, instance_of, optional  # SQLAlchemy-like

def positive(_, __, val):
    if val <= 0:
        raise ValueError

@attr.s
class A:
    a = attr.ib(validator=optional(and_(instance_of(int), positive)))

With a little bit of black magic and_ could probably generate and evaluate a combined validator with minimal overhead.

The syntax is clunky, but using & would require validators to subclass something I think?

[hynek]

Are you suggesting to make lists syntactic sugar for an and_() validator and simplify everything? I think I could get on board with that.

[Tinche]

Point 0: I see we already have an _And validator. I guess I forgot about it since it's new-ish. This should probably be moved into the validators module and we should probably use it in optional.

Point 1: It'd be possible to create a more efficient _And validator. I could put this together rather quickly if we want.

Point 2: We could expose the _And validator somehow, as a function. (and_?)

Point 3: Musings on syntax. Writing _and(instance_of(int), positive) is a little annoying. It'd be cool if we could write instance_of(int) & positive since I remember SQLAlchemy working a little like this. Also Hypothesis strategies can be combined using | which I find super nifty (numbers = integers() | floats()).

I don't think this is currently possible since the requirements for validators are that they are just ordinary functions.

Using lists as a syntax for and is interesting. I don't think we can make it work for arbitrary user validators though. Imagine I write for myself an either_of validator.

def either_of(*subvalidators):
    def inner(inst, attr, val):
        # Check all subvalidators, pass if at least one passes
    return inner

@attr.s
class A:
     a = attr.ib(validator=either_of([instance_of(int), positive], [instance_of(str)]))

So I want to validate a is either a positive int or a string. This won't work automatically since either_of would have to know to convert a list of validators into an _And validator somehow.

I suggest we go with points 0 and 1 for this release and bikeshed the rest later.

[hynek]

Point -1: 17.1 has to come out before PyCon. Realistically that means Saturday morning in my FOSS session.

Point 2: We could expose the And validator somehow, as a function. (and?)

Yes, I’m tending to use sqlalchemy vocabulary where possible.

I don't think this is currently possible since the requirements for validators are that they are just ordinary functions.

Yeah, while a neato syntactic sugar, let's not get carried away.

Using lists as a syntax for and is interesting. I don't think we can make it work for arbitrary user validators though. Imagine I write for myself an either_of validator.

No we can’t, but we can make the most common case nicer.

I suggest we go with points 0 and 1 for this release and bikeshed the rest later.

Yeah, let’s get 0 and 2 (just moving and exposing…) merged and do 1 in a separate PR.

[Tinche]

👍

[hynek]

Assuming tests will pass (Travis is quite drowsy today), how does this look?

[Tinche]

Will take a look in the evening.

[Tinche]

Serious question: can we live with an import at the end of a file?

This will allow us to:

1. Move _AndValidator to validators, where it belongs
2. Do this easily:

def and_(*validators):
    """
    A validator that composes multiple validators into one.

    When called on a value, it runs all wrapped validators.

    :param validators: Arbitrary number of validators.
    :type validators: callables

    .. versionadded:: 17.1.0
    """
    vals = []
    for validator in validators:
        vals.extend([validator] if not isinstance(validator, _AndValidator)
                    else validator._validators)
    return _AndValidator(vals)

What this does is:

and_(val1, and_(val2, val3)) == and_(val1, val2, val3)

which is especially useful for @validator use.

[hynek]

I don’t quite follow, how would @validator add another _AndValidator to an existing one? Can you give me an example please?

[Tinche]

I meant something like

@attr.s
class A:
    a = attr.ib(validator=[x, y])
    @a.validator
    def more_validation(_, __, ___):
        pass

It's not a big deal, but it'd also let us make _AndValidator frozen and remove the .add() on it elegantly.

[Tinche]

Sorry if I'm not being clear, I'm a little tired. I'm saying .add() on _AndValidator is unnecessary if and_() can just flatten. Instead of

validator = _and(x, y)
validator.add(z)

the equivalent is

validator = _and(x, y)
validator = _and(validator, z)

I asked for the import because it'd be useful to have access to this and_ in _make.py. Due to circular imports either we need to import and_ at the end of the file or move it and _AndValidator into _make.py I think.

[hynek]

I refuse to have circular imports but other than that, I believe I’ve realized your vision?

N.B. attrs will never have any actually frozen classes. If people want to do that to their own stuff: go for it. But attrs itself won't run with an engaged handbrake Unless you find a ways to make it just as fast one day. ;)

[Tinche]

Unless you find a ways to make it just as fast one day. ;)

That's the plan ;)

Anyway, I think this is good to go. Great work 💖