-
Notifications
You must be signed in to change notification settings - Fork 9
Add the ability to specify a config to validators #323
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
Conversation
stefanvanburen
left a comment
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.
looks reasonable to me, two minor things
tests/validate_test.py
Outdated
| assert str(vce.exception) == expected | ||
|
|
||
| # Test collect_violations | ||
| with self.assertRaises(protovalidate.CompilationError) as cvce: | ||
| v.collect_violations(msg) | ||
| assert str(cvce.exception) == expected |
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.
nit: use self.assertEqual to compare the exception strings? we're nearly fully on the unittest style of assertions, even though we're using the pytest runner, so makes sense to be consistent. 🤷
protovalidate/__init__.py
Outdated
| validate = _default_validator.validate | ||
| collect_violations = _default_validator.collect_violations | ||
|
|
||
| __all__ = ["CompilationError", "ValidationError", "Validator", "Violations", "collect_violations", "validate"] |
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.
do we need to from config import Config and add it to __all__ here so that consumers can use it?
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.
(eh, I guess not if we just do the from protovalidate.config import Config, ignore me maybe)
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.
Yeah I'm not sure if there's a benefit or not? I can definitely add it if you think it's worth it.
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 think it's somewhat common to include everything that's "public" re-exported in the __init__.py, so I think it's reasonable. so that callers can do:
from protovalidate import Validator, Configinstead of
from protovalidate import Validator
from protovalidate.config import ConfigThere 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.
ok, updated. i had to keep all the stuff internal to the protovalidate module though to use from protovalidate.config, otherwise i was seeing this:
ImportError: cannot import name 'Config' from partially initialized module 'protovalidate' (most likely due to a circular import)Unless you see something obvious I'm missing?
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.
that seems right, yep
Brings us full circle, effectively backing out #323. Resolves #362. [1]: https://docs.python.org/3/tutorial/controlflow.html#keyword-only-arguments
Brings us full circle, effectively backing out #323. Resolves #362. [1]: https://docs.python.org/3/tutorial/controlflow.html#keyword-only-arguments
This adds the ability to specify a
Configdata class to validators when creating them. Prior to this PR, validators could be created one of two ways:With this PR, it is now possible to create a validator with a
Configobject, specifying various options (onlyfail_fastfor now) for configuring a validator. To use a config, users must explicitly instantiate a validator and specify their config. They cannot use the module singleton as this only allows for a default validator.For example:
This PR is a breaking change. As a result of the above, the
fail_fastparameters have been removed from the signatures forvalidateandcollect_violations. Users should instead use the above config method to specifyfail_fast.This also adds a bit more depth to the
validate_testunit tests by testing creation of a default validator via the module and via explicit instantiation. It also tests that the violations returned bycollect_violationsand raises as part of the exception returned fromvalidateare the same.