Add new module for measurement data #299#313
Conversation
|
This module is ready for implementation. Please review @Cpprentice |
Cpprentice
left a comment
There was a problem hiding this comment.
Okay so first, this pull request contains a ton of changes that are not related to new modules and it further combines the changes for the module energy system and measurement data at many places.
I only focussed on the changes related to the measurement module.
Here are the things I found:
- CHANGELOG.md: the Changelog does not yet contain the addition of the measurement module
- oemetadata/v2/v21/metadata_key_details.md: the ontology classes are missing for the new properties - Do we use the predicates from the context json here?
- docs/oemetadata/metadata_key_description.md: same applies here
- docs/oemetadata/metadata_key_details.md: Does not have the keys for the measurement module yet
- oemetadata/v2/v21/metadata_key_details.md, oemetadata/v2/v21/metadata_key_description.md, docs/oemetadata/metadata_key_description.md, and oemetadata/v2/v21/build_source/schemas/module_measurement_data.json: The metadata key descriptions in v21 still use the required cardinality for moduleDescription and instrumentIdentifier. In my Opinion we should add least keep the moduleDescription as required if the module is used. The instrumentIdentifier can be optional.
|
I included the changes for this module. |
|
All entries of all modules are new and must be optional. |
|
Thanks for the reviews, please approve @jh-RLI @Cpprentice |
Cpprentice
left a comment
There was a problem hiding this comment.
From my view the inconsistencies have been resolved and I approve the pull request.
One remark though - the tests seem to catch their exceptions so they wont fail even on a validation error and the other test seems to be doing nothing. But I am not sure if that is relevant for the pull request.
Summary of the discussion
Add metadata keys for module section Measurement Data.
Type of change (CHANGELOG.md)
Added
Measurement Datafor dataset (#313)Workflow checklist
Automation
Closes #299
PR-Assignee
Reviewer