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

Add new base validator and base constraint classes #136

Closed
wants to merge 3 commits into from

Conversation

drmull
Copy link
Contributor

@drmull drmull commented Aug 28, 2020

Add new base classes to validator and constraint that take additional arguments, such as schema, path etc. This allows constraints to use non-primitive validators and resolves issue #133

My initial intention was to pull out the logic for the non-primitive validators from the schema file and put them in each respective validator class but then I figured it would be a bit too drastic..

drmull added 2 commits August 28, 2020 22:39
These base classes takes additional parameters such as the schema
object to allow the contraints to use non-primitive validators

Create new base classes for validator and constraint

remove dead code

Move optional check into base validator
@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request fixes 6 alerts when merging e179cda into d2d65eb - view on LGTM.com

fixed alerts:

  • 6 for __eq__ not overridden when adding attributes

@drmull drmull changed the title Add new base validator and contraint classes Add new base validator and base constraint classes Aug 28, 2020
for k in value.keys():
if self.key.validate(k) != []:
errors = schema._validate(self.key, k, DataPath(), strict)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to change the name of the _validate function, I will fix it

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request fixes 6 alerts when merging 5965dc6 into d2d65eb - view on LGTM.com

fixed alerts:

  • 6 for __eq__ not overridden when adding attributes

@mildebrandt
Copy link

Welcome back @drmull :)

FYI: This may take me a while to get to, it's a busy time over here. Sorry.

@drmull
Copy link
Contributor Author

drmull commented Aug 29, 2020

Welcome back @drmull :)

Thanks!

FYI: This may take me a while to get to, it's a busy time over here. Sorry.

No problem, I am in no rush :)

@mildebrandt
Copy link

Sorry for the long delay.

I grabbed the example from #133, but I couldn't get your code to work with it.

data:

maptest: 
  1: xpto 
  2: qwerty 
  error: wrong

schema:

maptest: map(include('value'), key=include('key')) 
--- 
value: str()
key: int()

This combination should fail validation.

@drmull
Copy link
Contributor Author

drmull commented Dec 1, 2020

Hm, strange. I updated the test case to match the example but I get:

maptest: Key error - 'error' is not a int.

Do you use the PyYAML parser? Maybe the order of the elements can differ with different parsers and trigger the problem.

I will take a closer look!

@mildebrandt
Copy link

mildebrandt commented Dec 2, 2020

Yeah, I'm using PyYaml 5.3.1.

However, I'm beginning to think this isn't something we should support. Allowing for include in the key constraint only makes sense when processing complex keys, for example:

? {one: 1, two: 2}
: [ 2001-01-01, 2002-02-02 ]

Both PyYAML and ruamel fail to load such a document, but it's valid yaml. Instead, I think the correct course of action is to not allow the include validator for the key constraint and raise an error saying something like:

The include validator is not valid for the key constraint.

Other validators can be used to constrain the key and still support all yaml documents that PyYAML and ruamel can load.

@drmull
Copy link
Contributor Author

drmull commented Dec 3, 2020

Yeah, I can agree with that. I will close this PR and we can reopen it if ruamel implements complex keys in the future 😁

@drmull drmull closed this Dec 3, 2020
@mildebrandt
Copy link

Cool, and thanks for your willingness to always jump in!

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.

None yet

2 participants