-
-
Notifications
You must be signed in to change notification settings - Fork 367
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 a not_ validator #1010
Add a not_ validator #1010
Conversation
Yeah sorry, this is a no-go. We can't just blanketly eat exceptions. Sorry for suggesting that! Feel free to submitting a not_in PR. |
I guess I'm not sure how it "blanketly eats" the exceptions. I view validators as effectively returning a boolean: As I mentioned, |
This is not a safe assumption to make – validators can raise any exception and the fact that our internal ones usually raise ValueErrors (I think there’s a TypeError in there too tho?) is not a general rule. Since attrs is not a validation library, but tries to help you write classes like you would, I encourage people to use any exception that makes sense to them. That said, and after thinking some more about it, I could live with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry for the detour. It’s almost there, this should be quick to merge.
bfab79a
to
1d02d37
Compare
Set the default captured exceptions to In order to validate them, I maybe went a bit overboard and modified the |
1ebd298
to
5e7d6be
Compare
src/attr/validators.py
Outdated
"not_ validator child '{validator}' did not raise " | ||
"a captured error".format(validator=repr(self.validator)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two things:
- I know we forgot for a few validators, but all validators should also raise at least the attribute as the second argument. Ideally also what it expects and what it got instead but I'm not sure if that makes any sense here – you could make the validator an arg tho!
- Should we maybe make the error message configurable? The current one can't be exposed to anyone using the class. E.g.
not_(instance_of(str), msg="x must not be a str.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll work on this over the weekend. I was just working from another validator's code, but I'll try to replicate a more recent one.
Was the subclass_of
stuff abhorrent, or OK to leave in (just using it for 'internal-use' validation)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with the subclass stuff if it stays internal!
d34b5bc
to
6aab304
Compare
Should be good for re-review. I have
I'm speccing it in the
which gets 'tested' as
Though I think my problem is I don't know how to specify that the type is The other is Sphinx complaining that my type declaration of |
076bed4
to
517fee1
Compare
Derp, fixed I think. Was doing |
Will fix. Sorry to 🚲🏠, but another edit I'm noticing as I'm looking at what to put in the exception args, various validators seem to have different ideas:
Should replicate the top as you suggested and have the docs look like the others, e.g.:
The 'expected' part should probably be the wrapped validator that should have failed, but is it worthwhile tacking on the captured errors? As |
6d69e5f
to
7e69dd7
Compare
7e69dd7
to
52c8ffb
Compare
Anything else to do? The only checkmark I'm not sure about is the hypothesis testing: none of the other validators have it, and when I started on it, it felt very much like I was reimplementing the same logic to derive what should be expected from various permutations received. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright thanks!
One minor nit that I'll fix myself: don't put default values into type stubs but a ...
.
Summary
Still a bit of a WIP.
My original motivation for this validator was to enable constructions like
not_(in_(["some", "reserved", "keywords"]))
, and rather than duplicating and swappingin
fornot in
in thein_
validator, figured I could make it composable.Pull Request Checklist
Our CI fails if coverage is not 100%.
.pyi
).tests/typing_example.py
.attr/__init__.pyi
, they've also been re-imported inattrs/__init__.pyi
.docs/api.rst
by hand.Changes to the signature of@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives.Find the appropriate next version in our
__init__.py
file..rst
files is written using semantic newlines.changelog.d
.