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

Some new validators #425

Merged
merged 21 commits into from Nov 8, 2018

Conversation

Projects
None yet
3 participants
@mattsb42-aws
Contributor

mattsb42-aws commented Aug 16, 2018

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. n/a
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • 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) have news fragments in changelog.d.

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!

Description

These are a few validators that I have ended up needing multiple times in my projects and I figured they would be generally useful. If you would prefer I split them up into three PRs, let me know. They're completely independent of each other.

  • Callable Validator : validates that a value is callable
  • Deep Iterable Validator : Allows recursion down into an iterable, applying another validator to every member in the iterable as well as applying an optional validator to the iterable itself.
  • Deep Dictionary Validator : Allows recursion down into the items in a dictionary, applying a key validator and a value validator to the key and value in every item. Also validates that the top level value is a dictionary using the existing instance_of validator.

TBD

Something I couldn't quite decide on an approach for was how to present the lower-level validator exceptions to the user.

For example, if you define the following:

@attr.s
class Example(object):
    special_iterable = attr.ib(validator=attr.validators.deep_iterable(attr.validators.instance_of(int)))

example = Example(["foo"])

How should the error message present the validation of "foo" in the error message?

I currently just have it allowing the lower level exceptions to bubble up directly. This is the simplest approach, but as implemented it makes the connection between the attribute and the value difficult to discern. This would be especially problematic if you had a more complex definition, such as:

@attr.s
class Example(object):
    complex_attribute = attr.ib(
        validator=attr.validators.deep_dictionary(
            key_validator=attr.validators.instance_of(str),
            value_validator=attr.validators.deep_iterable(
                member_validator=attr.validators.in_([1,2,3]),
                iterable_validator=attr.validators.instance_of(list)
            )
        )
    )

Thoughts?

(note: because of the above TBD, there are some hanging unused exception instance variables that the lint testenv will complain about for now)

@wsanchez

This comment has been minimized.

wsanchez commented Aug 16, 2018

@mattsb42-aws: what do you think about instead of "Deep Dictionary Validator", you change it to "Deep Mapping Validator". That would mean dropping the instance_of check, or adding a mapping_validator like you did in the iterable case?

I think that might be more generally useful, and makes the two more consistent.

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Aug 16, 2018

@wsanchez That sounds reasonable. I had been thinking in terms of builtin primitives, but you make a good point about generalized "things that implement __getitem__()".

To that end, I think I'll also change the iteration logic to remove the dependency on items() and instead use __iter__() and __getitem__().

mattsb42-aws added a commit to mattsb42-aws/attrs that referenced this pull request Aug 20, 2018

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Aug 20, 2018

Updates added to change to generalized deep_mapping.

Any thoughts on the error message handling?

@hynek hynek added this to the 18.2 milestone Aug 21, 2018

@hynek

This comment has been minimized.

Member

hynek commented Aug 21, 2018

Just a bunch of docs comments (I leave code to @wsanchez – can’t promise your PR will make it into the already very overdue 18.2 – sorry in advance):

Your news fragment renders really funky. I think it’s because of the indentation of the second line?

Also news fragment since I’m the only one who cares about this stuff ;):

And finally: the new APIs need to be added to https://github.com/python-attrs/attrs/blob/master/docs/api.rst – I should add this to the pull request template.

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Aug 22, 2018

Your news fragment renders really funky. I think it’s because of the indentation of the second line?

Gah..downside of switching back and forth between Markdown and RST...I always forget that RST wants exactly two (not four) spaces indent to join a multiline list entry.

You have a conflict in [news fragment file].

I assume that the expected filename is generally <issue/PR number>.change.rst but it looks like 425.change.rst was already taken by the events of PR #426. To what number/name would you prefer I rename it (and will the mismatch affect towncrier at all?)

I'll fix the other issues. Related to fixing docs stuff, if there's no objection to adding it, something I've found very useful on other projects is to have a tox env for hosting the built docs (ex[1], shamelessly copied from flake8). It just makes previewing built docs much simpler: just throw up tox -e docs,serve-docs and re-run/refresh as you change files.

[1] https://github.com/aws/aws-encryption-sdk-python/blob/master/tox.ini#L255-L261

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Aug 22, 2018

Requested docs changes made. I also went ahead and included the serve-docs environment, but I can drop that if you like.

The examples in the docs kind of highlight the issue I've been having coming up with a good way of formatting the error messages to identify the attribute that is failing validation.

Maybe rather than passing the original attribute to the sub-validator we could pass a modified copy with a different name?

ex:

class _DeepIterable(object):
    ...

    def __call__(self, inst, attr, value):
        """
        We use a callable class to be able to change the ``__repr__``.
        """
        if self.iterable_validator is not None:
            self.iterable_validator(inst, attr, value)

        member_position = 0
        for member in value:
            member_attr = copy(attr)
            member_attr.name = "{original}[{pos}]".format(original=attr.name, pos=member_position)
            self.member_validator(inst, member_attr, member)
            member_position += 1

@hynek hynek modified the milestones: 18.2, 18.3.0 Sep 1, 2018

@wsanchez

This comment has been minimized.

wsanchez commented Nov 6, 2018

CI failed for what looked like a transient error in the lint checks, so I restarted that and it failed with actual errors:

src/attr/validators.py:193:80: E501 line too long (83 > 79 characters)
src/attr/validators.py:222:80: E501 line too long (99 > 79 characters)
src/attr/validators.py:260:80: E501 line too long (90 > 79 characters)
src/attr/validators.py:271:80: E501 line too long (90 > 79 characters)
tests/test_validators.py:286:80: E501 line too long (96 > 79 characters)
tests/test_validators.py:310:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:321:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:326:80: E501 line too long (85 > 79 characters)
tests/test_validators.py:332:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:337:80: E501 line too long (85 > 79 characters)
tests/test_validators.py:344:80: E501 line too long (90 > 79 characters)
tests/test_validators.py:351:80: E501 line too long (100 > 79 characters)
tests/test_validators.py:358:80: E501 line too long (82 > 79 characters)
tests/test_validators.py:362:80: E501 line too long (106 > 79 characters)
tests/test_validators.py:399:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:411:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:422:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:433:42: F841 local variable 'e' is assigned to but never used
tests/test_validators.py:449:80: E501 line too long (105 > 79 characters)
@wsanchez

Code looks good other than linter failures that need fixing.
I'd remove the tox.ini edit unless @hynek is OK with it.

tox.ini Outdated
changedir = docs/_build/html
commands =
python -m http.server {posargs}

This comment has been minimized.

@wsanchez

wsanchez Nov 6, 2018

I have no opinion as to whether this is a good addition, but it's unrelated to the PR, so probably should be done separately, otherwise it's kind of sneaking in the back door. @hynek ?

This comment has been minimized.

@mattsb42-aws

mattsb42-aws Nov 6, 2018

Contributor

Fair enough. I have since discovered sphinx-autobuild, which is a much better solution to the problem overall anyway.

@wsanchez wsanchez added the Feature label Nov 6, 2018

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Nov 6, 2018

Code looks good other than linter failures that need fixing.

@wsanchez any thoughts on the error message contents discussed above?

@mattsb42-aws mattsb42-aws force-pushed the mattsb42-aws:new-validators branch from ef9affd to 2f39783 Nov 8, 2018

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Nov 8, 2018

Linting cleanup and revert of the tox.ini change. I also rebased off the current master (no conflicts) because git was upset.

@mattsb42-aws

This comment has been minimized.

Contributor

mattsb42-aws commented Nov 8, 2018

I went ahead and just went with the the error messages as-is because I couldn't figure out a good way to duplicate an attribute with a different name. This will make debugging an error message harder than I would like, but it's a starting point.

@wsanchez

This comment has been minimized.

wsanchez commented Nov 8, 2018

I went ahead and just went with the the error messages as-is because I couldn't figure out a good way to duplicate an attribute with a different name. This will make debugging an error message harder than I would like, but it's a starting point.

Agreed. This is a good start and we can fix that when we figure out a good way to do it.

Thanks for the contribution!

@wsanchez wsanchez merged commit 336d052 into python-attrs:master Nov 8, 2018

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek

This comment has been minimized.

Member

hynek commented Nov 8, 2018

Yes thank you very much! Your contribution caught me in an unfortunate time, I’m glad Wilfredo saw it thru!

@mattsb42-aws mattsb42-aws deleted the mattsb42-aws:new-validators branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment