Skip to content
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

Support for oneOf composition #132

Closed
wants to merge 4 commits into from
Closed

Conversation

maroux
Copy link
Contributor

@maroux maroux commented Dec 9, 2016

Fixes #131

@maroux
Copy link
Contributor Author

maroux commented Dec 9, 2016

Not sure how flex/loading/common/pattern.py is failing flake8 on master, regardless I just decided to fix it as part of this :/

@pipermerriam
Copy link
Owner

Not sure how flex/loading/common/pattern.py is failing flake8 on master, regardless I just decided to fix it as part of this :/

Likely a new version of flake8 with updated or new rules. Thanks for fixing.

Copy link
Owner

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the two minor bits of feedback. If you can address those I'll get this merged and released.

it = iter(iterable)
# check that one is True, then check that none is True in rest of the
# iterator
return any(it) and not any(it)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really clever. Enough so that I'd like to request that you write a set of simple test cases that demonstrate it works because it's not entirely intuitive at a glance how it works. I do however like this pattern alot.

@@ -299,7 +300,7 @@ def generate_enum_validator(enum, **kwargs):


@skip_if_empty
def validate_allof_anyof(value, sub_schemas, context, method, **kwargs):
def validate_composition(value, sub_schemas, context, method, default_error_message=None, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think error_message_override might be a more appropriate name than default_error_message. Alternatively I'd support removing this in place of just writing 3 small functions which each raised the appropriate error message for all/any/exactly_one_of so that this function doesn't have to expand it's signature. Your call.

@pipermerriam
Copy link
Owner

@maroux If you you can hit the two small requests for cleanup I'll get this merged. Also it appears this now needs to be rebased.

@0181532686cf4a31163be0bf3e6bb6732bf

Any change of merging this?

@0181532686cf4a31163be0bf3e6bb6732bf

I can make the cleanup commits if @maroux don't mind

@0181532686cf4a31163be0bf3e6bb6732bf

@pipermerriam @maroux What do you think?

@maroux
Copy link
Contributor Author

maroux commented Nov 23, 2017 via email

@pipermerriam
Copy link
Owner

@lyssdod please do. Ping me when this is ready for review. I'm closing this issue so you can open your own PR (which it appears you've already done)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants