-
-
Notifications
You must be signed in to change notification settings - Fork 374
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 attrs.validators.or_
validator
#1303
Conversation
CodSpeed Performance ReportMerging #1303 will not alter performanceComparing Summary
|
Although good warning in general, in this particular code, they do not fit here. * BLE001 Do not catch blind exception: `Exception` `validators` usually raise `ValueError` upon their violation. However, it's not the only `Exception` they can raise - cf. `attrs.validators.not_` - therefore, it is desirable to catch them all! * PERF203 `try`-`except` within a loop incurs performance overhead Fair point, but the loop is written in a way to short-circuit, ie. it will finish when a first validator is satisfied. * S112 `try`-`except`-`continue` detected, consider logging the exception Not applicable here, we care only if **all** validators raise an exception, which is already accomodated for after the `for` loop.
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.
code's looking good; just a few smaller issues pls
docs/api.rst
Outdated
>>> from typing import Union | ||
>>> @define | ||
... class C: | ||
... val: Union[int, str] = field(validator=attrs.validators.or_(attrs.validators.instance_of(int), attrs.validators.instance_of(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.
this is not a great validator because, and I may blow your mind here ;), is instance takes a tuple! So you could write this: attrs.validators.instance_of((int, str))
or even attrs.validators.instance_of(int | str)
on modern Pythons.
Just make up something with numbers I guess?
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.
Yep, I was aware of the fact you can can isinstance
with type argument being a tuple, but didn't put 2 and 2 together that it will be the case with attrs.validators.instance_of
as well. 🤦🏻♂️
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.
Done in a0b6b24 .
I reworked the example to have one-or-many
scenario, where the attribute can either a value of a type or a collection of values of the same type, encountered eg. in parsing the query parameters in HTTP URIs.
...?val=1&val=2&val=3
Is this OK?
As an aside, docs
seem to be tested wth py312, so I don't have to do the typing
ceremony and can write val: int | list[int]
, but since there are python versions in the test matrix that don't have this syntax on by default, should I keep it as it is now or rework to conform to newer versions?
Thanks!
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.
Yes, please use the most-modern syntax. Let’s teach ppl modern features.
Example looks fine on a phone 😅
Co-authored-by: Hynek Schlawack <[email protected]>
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!
No problem, pleasure on my side for being able to contribute to a library I consider one of the foundational for the whole Python ecosystem! I thank you for timely responses and guidance. Cheers, |
Closes #1214
Summary
Add
attrs.validators.or_
validator, which is satisfied if at least one of the validators it is wrapping is satisfied, thus complementing theand_
andnot_
"logical connectives" validators.Pull Request Check List
main
branch – use a separate branch!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.@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives.The next version is the second number in the current release + 1.
The first number represents the current year.
So if the current version on PyPI is 22.2.0, the next version is gonna be 22.3.0.
If the next version is the first in the new year, it'll be 23.1.0.
.rst
and.md
files is written using semantic newlines.changelog.d
.TODO (done!)
changes
dir, but that is dependent on the PR number of this PR, so needs to be post hoc - done in 6996627docs/api.rst
- done in 2ad53b3