-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #17 issue, adding ObservableApplicationVersion class, changing Ap… #58
Conversation
…nging ApplicationFacet, changing Bundle classes
@ajnelson-nist A slightly better solution would be to avoid the statement: self["uco-core:UcoInherentCharacterizationThing"] = [] and change the append_to_uco_inherent_characterization_thing method like this: if not self.get("uco-core:UcoInherentCharacterizationThing"):
self["uco-core:UcoInherentCharacterizationThing"] = []
self._append_observable_objects(
"uco-core:UcoInherentCharacterizationThing", *args
) |
The Python class hierarchy in this code base, before this patch, is rooted with: ``` FacetEntity (corresponding with uco-core:Facet) - ObjectEntity (correspinding with uco-core:UcoObject) ``` UCO's class hierarchy starts with: ``` UcoThing - UcoInherentCharacterizationThing - Facet - UcoObject ``` This patch reimplements the Python class hierarchy's root to align to the ontology. Some other changes will be necessary, but this patch implements the minimal required by type review. No effects were observed on Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
No effects were observed on Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
No effects were observed on Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
ApplicationVersion was being treated as a UcoObject, and being appended to the Bundle. The `_append_stuff` method instead needed to be called with `objects=True` in order to inline the nodes. Alternatively, the generated JSON-LD object could be given a `"@graph"` key, instead of housing everything in a Bundle. This seems like it will be necessary by the time shared object-references are added, e.g. with `uco-types:Hash`. A follow-on patch will regenerate Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
Signed-off-by: Alex Nelson <[email protected]>
@@ -415,6 +425,52 @@ def __init__(self, *args: Any, has_changed=None, state=None, **kwargs: Any) -> N | |||
self._bool_vars(**{"uco-observable:hasChanged": has_changed}) | |||
|
|||
|
|||
class FacetApplication(FacetEntity): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review note: This class definition needed to move below ObservableObject
's definition for a type-signature reference.
A follow-on patch will regenerate Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
No effects were observed on Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reviewing the newly-generated JSON-LD data, I saw reason to make significant revision to the base class hierarchy and port some prior implementation into generic classes in types.py
.
The extra implementation suggests some other changes that should be just outside the scope of this PR:
- There are some other opportunities to extend the
types.Dictionary
class to cover prior generated example data related to EXIF. types.Hash
should also be implemented so we can decide some code-design matters around when to fully inline nodes vs. make a stub reference.
In light of the scope-creep, I think someone else should review and merge this PR now.
@ajnelson-nist I changed the Bundle class in core module, by defining a new "property" self["uco-core:UcoInherentCharacterizationThing"] = [] and adding the append_to_uco_inherent_characterization_thing(self, *args) method. Therefore instead of defining the super classes (UcoThing and UcoInherentCharacterizationThing) I thought that the changes I made could be a simpler solution. If you think it would be better to adopt another solution let me know, I will gladly do so!