Add ability to assert metadata properties on input dataset parameters - #15825
Conversation
cb03665 to
b866ed3
Compare
| in_range=InRangeValidator, | ||
| length=LengthValidator, | ||
| metadata=MetadataValidator, | ||
| dataset_metadata_equal=MetadataEqualsValidator, |
There was a problem hiding this comment.
Maybe metadata_equal for consistency with the above type?
What about stronger typing. I feel like I've been heading this way with tool stuff... if value is used treat it as a string and use a value_json attribute to json.load the value for a typed comparison.
I would make these two changes but you write more tools - if you prefer the way you have it here that is totally fine with me.
There was a problem hiding this comment.
Maybe
metadata_equalfor consistency with the above type?
Hmm. We also have dataset_metadata_... maybe we can simplify/unify all this by adding a few aliases and name all of them metadata_...?
What about stronger typing. I feel like I've been heading this way with tool stuff... if
valueis used treat it as a string and use avalue_jsonattribute tojson.loadthe value for a typed comparison.
value_json is a great idea. I vaguely remembered that I have seen something like this but was not sure. I will add it.
I would make these two changes but you write more tools - if you prefer the way you have it here that is totally fine with me.
Thanks for the feedback. Give me a moment :)
There was a problem hiding this comment.
While thinking about this I would suggest to stick with dataset_metadata_equal: All the user (ie tool developer) facing validators are prefixed with dataset_metadata_. The metadata validator is unlikely to be used by tool developers (its automatically added to each data parameter). So maybe this is an acceptable level of inconsistency...
1bc8dd0 to
0e059f8
Compare
|
This needs a pass of black. |
|
Good to go? |
|
This PR was merged without a "kind/" label, please correct. |
| super().validate(isinstance(value, model.DatasetInstance) and not missing, value_to_show=missing) | ||
|
|
||
|
|
||
| class MetadataEqualsValidator(Validator): |
There was a problem hiding this comment.
I'm late, but maybe MetadataEqualValidator instead of MetadataEqualsValidator for consistency?
revive #3159 from @jmchilton (anno 2016 .. just a little bit more than crazy 10K PRs/issues in between :))
Changed the name for the validator ..
datasetprefix might be good for comparability.Not sure if one could / should generalize / reuse
dataset_metadata_in_range(which considers only numbers at the moment).ping @bgruening
Side note: I would find this super userful to check for column names, but
tabulardoes not set them .. still downstream tools could and et voila it can be used :).How to test the changes?
(Select all options that apply)
License