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

list of validator #158

Closed
wants to merge 2 commits into from
Closed

list of validator #158

wants to merge 2 commits into from

Conversation

reece
Copy link

@reece reece commented Feb 27, 2017

From the new docs:
"A validator that raises a TypeError if the initializer is called with a non-list type or with a list that contains one or more elements of a wrong type. None values are not permitted. As with instance_of, checks are perfomed using isinstance() therefore it’s also valid to pass a tuple of types."

@codecov
Copy link

codecov bot commented Feb 28, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master   #158   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         551    563   +12     
  Branches      122    125    +3     
=====================================
+ Hits          551    563   +12
Impacted Files Coverage Δ
src/attr/validators.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 f232cc8...764eed9. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 28, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master   #158   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         551    563   +12     
  Branches      122    125    +3     
=====================================
+ Hits          551    563   +12
Impacted Files Coverage Δ
src/attr/validators.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 f232cc8...764eed9. Read the comment docs.

@hynek
Copy link
Member

hynek commented Feb 28, 2017

OK so let’s do this.

But if we tackle this, I would suggest: container_of(container_type, element_type, empty_ok=True) with a bunch of convenience helpers list_of(), set_of(), tuple_of()

@reece
Copy link
Author

reece commented Feb 28, 2017

empty_ok=True means that the container can be empty, right?

What about also adding none_ok, permitting None values? Or could optional() be used for elements?

What about mappings? For example: mapping_of(container_type, key_type, value_type, empty_ok=True, none_ok=True)

@hynek
Copy link
Member

hynek commented Mar 4, 2017

empty_ok=True means that the container can be empty, right?

yep!

What about also adding none_ok, permitting None values? Or could optional() be used for elements?

I’d say that exactly what optional() is for. :) So yes, you’ll have to handle that case.

That reminds me: unless someone has good arguments, I don’t think we should do element_type but validator! And validator needs also to take a list. :)

What about mappings?

Baby steps. :)

@reece
Copy link
Author

reece commented Mar 5, 2017

I see what you did there with the "you'll need to..." !

I appreciate attrs, but I'm not up for implementing this now. You might prefer to close this PR and migrate comments to #39.

@hynek
Copy link
Member

hynek commented Mar 5, 2017

No problem! This is kind of the reason, why we still don’t have it: it’s more work than it looks initially if you want to cover all bases. :-/

@wsanchez
Copy link

wsanchez commented Mar 8, 2017

This feels like a bit of reinventing the type annotation system in Py3.

If I understand this correctly, the feature is validator=list_of(int).

Why not validator=instance_of(List[int])?

That is, I think I'd rather write this code:

from pathlib import Path
from typing import Iterable

from attr import attrib, attrs
from attr.validators import instance_of


@attrs()
class ThingWithPaths(object):
    _paths = attrib(validator=is_type(Iterable[Path]))

    def exists(self):
        for path in self._paths:
            if not path.exists():
                return False
        return True

thing1 = ThingWithPaths([Path("."), Path("foo")])
print("thing1 exists:", thing1.exists())

thing2 = ThingWithPaths([".", "foo"])  # validation fails here
print("thing2 exists:", thing2.exists())

So what's missing is is_type, instead of a series of foo_of validators.

Another advantage here is that if we have can find the type annotation (eg #151), then is could even be simpler: _paths = attrib(validator=typing) where the type for the validator to check is obtained from the annotations.

Copy link

@wsanchez wsanchez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IlyaSkriblovsky
Copy link

IlyaSkriblovsky commented Mar 9, 2017

@wsanchez, typing doesn't work for dynamic checking using isinstance:

>>> isinstance(['qwe', 'asd'], List[int])
True

typing is a declaration-only lib, it doesn't provide any means of runtime typechecking.

@wsanchez
Copy link

wsanchez commented Mar 9, 2017

@IlyaSkriblovsky Yeah, I'm figuring that out, which is weak sauce but that's why I changed the code to is_type from instance_of. It wasn't meant to be a thing that already works so much as what would be more awesome. :-)

I'll add here a reference to a ticket to fix that, which you just sent to Glyph: python/typing#377

I guess it's just unfortunate not to be able to leverage typing better. From your work on cattrs, I got the impression that it was possible, but that was optimistic, I guess.

@hynek
Copy link
Member

hynek commented Mar 21, 2017

So my understanding is that is_type with py3 syntax is not feasible anytime soon? In any case, as much as I loath Python 2, I’m sure a Python 3-only option would not stop people opening an issue and asking for this every week. :)

So I guess we still need a solution for all the legacy-impaired.

Closing this at the request of the reporter tho.

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.

4 participants