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
Implements excludes
#137
Implements excludes
#137
Conversation
Excellent. |
@@ -758,6 +758,17 @@ def _validate_valueschema(self, schema, field, value): | |||
if len(validator.errors): | |||
self._error(field, validator.errors) | |||
|
|||
def _validate_excludes(self, exclude, field, value): | |||
targets = [] | |||
if isinstance(exclude, list): |
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.
please test for isinstance(exclude, Sequence) and not isinstance(exclude, _str_types)
please also add a check to otherwise, lgtm. and i'm even baffled that |
... 'that_field': {'type': 'dict', | ||
... 'excludes': 'this_field'}} | ||
>>> v.validate({'this_field': {}, 'that_field': {}}, schema) | ||
... False |
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 has to be
False
also true for all following output of returned objects. with leading dots it's interpreted as statement, not as expected result.
Thank you very much for the review. I considered your observations and pushed the appropriate changes. However, I am not quite sure what you mean by
I added a test for the error string. Please elaborate if this was not what you meant. |
ah, sorry. i meant |
though i don't see a use-case for it, you might add handling of the rule for unknown fields for the sake of completeness. that would let any arbritary field exclude one or more defined fields. |
@@ -758,6 +758,16 @@ def _validate_valueschema(self, schema, field, value): | |||
if len(validator.errors): | |||
self._error(field, validator.errors) | |||
|
|||
def _validate_excludes(self, excludes, field, value): | |||
if isinstance(excludes, _str_type): |
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.
should test for Hashable
as any hashable object can be a mapping's key.
Could you please rebase this one of top of current |
Also pass flake8 test
They were interpreted as statement, not expected result
Namelly : - be consistent : name parameter `excludes` instead of `exclude` - if `excludes` is a string, simply wrap it in a list - test if `excludes` is a string with `_str_type`
Also tests
Rebased. Running
which are not consistent with the results expected by the test suite. Any idea how to fix those ? |
in such cases i adapt the doctests as unittests to investigate. |
For line 654:
|
The unittest succeeds because |
It looks like when both fields are required, the validatiion always fails. That makes sence, but the following test case should fails.
in particular the following lines :
Actually, the test suite still succeed if I changed those |
Yup see my comment on |
This makes sense. Do you have a suggestion about it ? I am about to patch the |
Patching |
Save a set of required fields, and ensure at least one of them is present in the end
0aa7aac
to
7504fc8
Compare
Please have a look at the latest commit : running
|
Thanks for the review once again. I applied your suggestions. |
@@ -818,6 +830,28 @@ def _validate_valueschema(self, schema, field, value): | |||
if len(validator.errors): | |||
self._error(field, validator.errors) | |||
|
|||
def _validate_excludes(self, excludes, field, value): |
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.
one thing i missed, within their functional grouped sections methods are sorted alphabetically. thus, this should be moved to line 578.
Keep definition validation method sorted alphabetically
Thanks! |
PS: feel free to PR to replace your nickname if so you wish (could not get a hold of your name from the logs). Or I can edit it for you :) |
great, thank you! |
Thanks for all the reviews and the approval. |
As suggested in #132.