Add ability to assert metadata properties on input dataset parameters #15825
Conversation
bernt-matthias
force-pushed
the
validation_metadata
branch
2 times, most recently
from
cb03665
to
b866ed3
Compare
| @@ -949,6 +986,7 @@ def validate(self, value, trans=None): | |||
| 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...
bernt-matthias
force-pushed
the
validation_metadata
branch
from
1bc8dd0
to
0e059f8
Compare
|
This needs a pass of black. |
|
Good to go? |
|
This PR was merged without a "kind/" label, please correct. |
| @@ -559,6 +560,42 @@ def validate(self, value, trans=None): | |||
| 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?
There was a problem hiding this comment.
Not to late #16489 :)
Thanks!
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