23andMe / Yamale

A schema and validator for YAML.
MIT License
680 stars 88 forks source link

Add new base validator and base constraint classes #136

Closed drmull closed 3 years ago

drmull commented 4 years ago

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..

lgtm-com[bot] commented 4 years ago

This pull request fixes 6 alerts when merging e179cda4bfaf3a0faabfd6d090658686808f2ca8 into d2d65eb513e5ae56fc73f888cbfaeb24a1e75b07 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

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

fixed alerts:

mildebrandt commented 4 years ago

Welcome back @drmull :)

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

drmull commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

mildebrandt commented 3 years ago

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