Aggregation Descriptor Ontology Validator Update #373
Conversation
Added the subfield category `aggregation_descriptor` to the ontology validator.
| @@ -220,6 +220,8 @@ def __init__(self): | |||
| _CATEGORIES_IN_ORDER = [ | |||
| _CAT_SPEC( | |||
| cat=subfield_lib.SubfieldCategory.AGGREGATION, required=False, max=1), | |||
| _CAT_SPEC( | |||
| cat=subfield_lib.SubfieldCategory.AGGREGATION_DESCRIPTOR, required=False, max=10), | |||
There was a problem hiding this comment.
Is there a need for allowing up to 10 aggregation descriptors in one field? From what I recall, the descriptors will only represent duration, and those should all be mutually exclusive.
There was a problem hiding this comment.
i included this for future compatibility, in case we need something along the lines of rolling or fixed window descriptors. this would give us that ability.
i believe our long-term plan for adding this in to the translation as an attribute might not work, so this would be the alternative.
| @@ -24,6 +24,7 @@ | |||
| from yamlformat.validator import subfield_lib | |||
|
|
|||
| AGGREGATION = subfield_lib.SubfieldCategory.AGGREGATION | |||
| AGGREGATION_DESCRIPTOR = subfield_lib.SubfieldCategory.AGGREGATION_DESCRIPTOR | |||
There was a problem hiding this comment.
We should add and/or modify field tests to use the new subfield category.
There was a problem hiding this comment.
i agree. what should this test be? thinking we should check that there is no agg_desc without an asssociated agg. does that make sense? if so, might need your help to expedite the test writing.
There was a problem hiding this comment.
That actually sounds like something we should add as a validation check, and the same for measurement descriptors.
The GitHub app is giving me conflicting info on whether or not I can push directly to this branch, so if you want me to take over writing tests, I may need to do it on a separate PR.
tools/validators/ontology_validator/yamlformat/tests/subfield_lib_test.py
Show resolved
Hide resolved
| @@ -144,8 +147,7 @@ def testAddFromConfig(self): | |||
| sff = subfield_lib.SubfieldFolder(_GOOD_PATH) | |||
| sff.AddFromConfig([doc], '{0}/file.yaml'.format(_GOOD_PATH)) | |||
| ns = sff.local_namespace | |||
|
|
|||
| self.assertCountEqual(['agg', 'comp', 'desc', 'mdesc', 'meas', 'ptype'], | |||
| self.assertCountEqual(['agg','aggdesc', 'comp', 'desc', 'mdesc', 'meas', 'ptype'], | |||
There was a problem hiding this comment.
| self.assertCountEqual(['agg','aggdesc', 'comp', 'desc', 'mdesc', 'meas', 'ptype'], | |
| self.assertCountEqual(['agg', 'aggdesc', 'comp', 'desc', 'mdesc', 'meas', 'ptype'], |
This line is too long, but I'm not sure what the preferred or linter-approved way to split it is.
There was a problem hiding this comment.
ive tried a backslash linebreak, but @charbull will need to confirm this works.
Added two new tests for aggregation descriptors (check there isn't more than one, check that it comes with an aggregation). Some minor cleanup for linter.
Changed the code so that aggregation_descriptor precedes aggregation.
Moved the
…ttps://github.com/google/digitalbuildings into Aggregation-Descriptor-Added-To-Ontology-Validator
charbull
deleted the
Aggregation-Descriptor-Added-To-Ontology-Validator
branch
Added the subfield category
aggregation_descriptorto the ontology validator.