-
Notifications
You must be signed in to change notification settings - Fork 22
Fix Issue #153; add missing labels #156
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
Conversation
(And correct its superclass)
'goal' showed up as a property with no label b/c it was never meant to be a property. it was a typo. correct the real problem
Fix it in 1 place, gistTop, which all others will import
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.
We'll need to mention the goal
/hasGoal
change in the restriction in the release notes, since it is functionally different (even though it just corrects an obvious problem, so still patch-level).
Yes @sa-bpelakh . I guess when we know which commits/PRs make the cut for the next release, and we have a release branch for it, we'll need to edit the OntologyFiles/ReleaseNotes.txt file on that branch with the appropriate notes. @rjyounes , perhaps that needs a place in some release checklist you may be developing for SA? |
I definitely do not remember agreeing to sentence case for labels. And a quick search of the notes in #20 show nothing like that.
I, for one, do not like the idea of sentence case labels. We should resolve this issue quickly.
From: Rebecca Younes <notifications@github.com>
Sent: Monday, January 27, 2020 7:59 AM
To: semanticarts/gist <gist@noreply.github.com>
Cc: Dan Carey <dan.carey@semanticarts.com>; Review requested <review_requested@noreply.github.com>
Subject: Re: [semanticarts/gist] Fix Issue #153; add missing labels (#156)
@rjyounes requested changes on this pull request.
________________________________
In OntologyFiles/gistIoT.owl<#156 (comment)>:
@@ -61,6 +61,8 @@
</owl:Class>
<owl:Class rdf:about="&gist;ControllerType">
+ <rdfs:subClassOf rdf:resource="&gist;Category"/>
+ <rdfs:label rdf:datatype="&xsd;string">Controller Type</rdfs:label>
We agreed on sentence case rather than title case for all labels - thus "Controller type" rather than "Controller Type." See discussion on issue #20<#20> - sorry, I forgot to copy over our decisions when I split up the issues.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#156?email_source=notifications&email_token=ABDGWJGTZ5RKD3POTF674UDQ73K7RA5CNFSM4KLUZFB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTD5TZA#pullrequestreview-348641764>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABDGWJBOZJSIPRWDHEYLTB3Q73K7RANCNFSM4KLUZFBQ>.
|
@DanCarey404 I am copying your comment to #20, which is where the original discussion took place. Let's move the discussion to that issue. |
|
||
<owl:Class rdf:about="&gist;ControllerType"> | ||
<rdfs:subClassOf rdf:resource="&gist;Category"/> | ||
<rdfs:label rdf:datatype="&xsd;string">Controller type</rdfs:label> |
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 change is a bit confusing, but on careful inspection, on each commit, its fine.
</owl:Class> | ||
|
||
<owl:Class rdf:about="&gist;Equipment"> | ||
</owl:Class> | ||
|
||
<owl:Class rdf:about="&gist;MessageDefinition"> | ||
<rdfs:subClassOf rdf:resource="&gist;SchemaMetaData"/> | ||
<rdfs:label rdf:datatype="&xs;string">Message Definition</rdfs:label> | ||
<rdfs:comment rdf:datatype="&xs;string">Each pulse from a Sensor is reflected in a message, as well as each instruction to an Actuator</rdfs:comment> | ||
<rdfs:label rdf:datatype="&xsd;string">Message Definition</rdfs:label> |
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.
gist has always used xs not xsd. Making this change should not be done here. It should be first put up as an issue to discuss, and if approved, make the change across all files.
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.
The serializer changed it. Or protege. I fought with it for a while, but then decided to do the format change as 1 commit, and the real fixes next, as otherwise I was at an impasse. The reformat should be a 1 time thing if we use the same serializer going forward.
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.
Should not be changing xs to xsd here. Should be raise as an issue and go through normal process. All gist files have always used xs, not xsd. We can decide to change that if we wish.
I find this a strange point. The use of a prefix is file-based and has no formal significance. It could be called xyz as long as it points to the right URI. The prefix itself has no consequence and no semantic value. xsd seems to me to be more commonly used. |
It's primarily convention, right? If I am going to edit the file, I don't want to have to scroll back to the prefix declarations for commonly used namespaces. I could name |
I've always used xsd as well. Interesting that the standard uses the prefix xs when the uri itself has "xsd-namespace." |
¯\_(ツ)_/¯ |
I also prefer xsd. |
@sa-bpelakh At first I thought that was a pushme-pullyou (are you familiar with Dr Doolittle?), which would have been equally applicable. |
@marksem Re release notes: Boris is responsible for these as he is our release facilitator for the January release. We probably want to come up with a template for these at some point. |
I'd like to propose the following so we can merge this PR:
Apparently it was rdf-toolkit that changed the prefix, so if we're going to use it we have to opt for xsd. How did we avoid that in the past? |
I'm happy with @rjyounes 's suggestion. |
The reason I suspected the serializer was that the prefix changes occurred in the commit that was only reformatting with the serializer. PLEO files are ttl; I wonder if the serializer behavior is different between the file types. It seems unlikely but I don't know how else to explain it. |
For pragmatic reasons, I disagree. It is now in the form that serializer wants. And that form uses xsd, our preferred ns. To put it back means more maintenance next time. We need to be avoiding changes that add maintenance time. |
Technically we have enough approvals to merge, but I don't want to act by fiat. We should be able to merge this and continue the discussion of label formats in issue #20. |
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.
@marksem There are merge conflicts because the changes from Steve's recent PR, which has been merged to develop, need to be pulled in. His were all typos that I assume you can just accept. Once this is done I'll approve and merge.
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 rdf:datatype="&xsd;string">Area</rdfs:label> | ||
<rdfs:comment rdf:datatype="&xsd;string">A measurement of two-dimensional space.</rdfs:comment> | ||
<owl:equivalentClass> | ||
<owl:Class> |
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.
So the transition I am seeing in several places here from
gist:SomeClass owl:intersectionOf ( FirstClass, SecondClass ) .
to
gist:SomeClass owl:equivalentClass [
a owl:Class;
owl:intersectionOf ( FirstClass, SecondClass ) .
]
Is it because the original declaration was never compliant and we are fixing it? Or are both forms valid and we are just choosing a preferred form?
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.
IIRC there are a bunch of these owl:intersectionOf constructs in gist that the old E6 tool wrote out that way but that Protege will rewrite into owl:equivalentClass (as above).
We (devs) tried to decide if the OWL spec requires it or not. I'd be interested in a more official answer from anyone that wants to go interpret the OWL spec.
Fixed merge conflicts. Will address formatting of label in #20. |
@Jamie-SA @sa-bpelakh I've had Protégé play this trick on me as well. It seems like the simpler version should be valid OWL and that the other is more verbose than necessary. Another reason not to like Protégé. |
If possible, just do what the serializer does. What Protégé or E6 do should not matter. |
Fixes #153
Had to do 1 re-serialize commit first, so that the "real" changes would be easy to see. See latter 3 commits for the actual changes. Note that the fix to labeling property
goal
was to fix the fact that it was a typo in theArtifact
restriction.