-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add --copy-annotation option to ontoconvert #732
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
==========================================
+ Coverage 72.12% 72.21% +0.08%
==========================================
Files 17 17
Lines 3444 3462 +18
==========================================
+ Hits 2484 2500 +16
- Misses 960 962 +2 ☔ View full report in Codecov by Sentry. |
ontopy/utils.py
Outdated
"""In all classes and properties in `onto`, copy annotation `src` to `dst`. | ||
|
||
The `src` and `dst` can either be provided as a label string or a full IRI. |
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.
I think we should document with "Parameters" of "Arguments", as this renders nicely in the docementaion
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.
done
if onto.world[src]: | ||
src = onto.world[src] |
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.
Why check in world?
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.
Because we want to support src of the form http://www.w3.org/2004/02/skos/core#prefLabel
if "://" not in dst: | ||
raise ValueError( |
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.
I think we should have tests for errors that we expect may happen.
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.
Isn't this such a test? True, there might exists invalid IRIs that contain "://", but this easy and covers most of all error cases. The rest will raise an exception from owlready2 when trying to create a new annotation property.
What other errors would you expect?
ontopy/utils.py
Outdated
name = min(dst.rsplit("#")[-1], dst.rsplit("/")[-1]) | ||
iri = dst | ||
dst = onto.new_annotation_property(name, owlready2.AnnotationProperty) | ||
dst.iri = iri |
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.
Needs test
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.
This is already tested in tests/tools/test_ontoconvert.py
Just codecov that doesn't recognise tests done in subprocesses...
@@ -39,3 +39,18 @@ def test_run() -> None: | |||
assert re.search("@prefix : <https://w3id.org/ex/testonto#>", output2) | |||
assert re.search("<https://w3id.org/ex/testonto> .* owl:Ontology", output2) | |||
assert re.search("testclass .* owl:Class", output2) | |||
|
|||
# Test 3 - copy-annotation |
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.
you are checking copy_preflabel. I agree that this is an alias for annotation, but I think it would be good to check that other annotations also are added. Most particularly, that we can copy to an annotation that is originally not in the ontology.
Also, I think we should have a test where preflabel is not converted, as prefLabel is copied by default (if I understand store_true correctly).
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.
rdfs:label is not in the ontology, so we are actually creating a new annotation.
Ok, added new test with src="prefLabel"
.
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.
Nice
Description
This option is needed to automatically rdfs:label annotations to classes and properties when publishing EMMO to increase the FAIRness or the ontology.
Type of change
Checklist
This checklist can be used as a help for the reviewer.
Comments