-
Notifications
You must be signed in to change notification settings - Fork 30
Issue 391: support for collections #403
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
base: gh-pages
Are you sure you want to change the base?
Conversation
Marking as draft whilst cleaning up the HTML to ask the following questions:
|
I would allow it for NodeShapes too. Your definition already uses the term value node, and for node shapes that just means the focus node. Are you hesitating because it may require additional test cases? And on RDF lists, why not just reference SHACL Lists within the same document? |
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.
tiny
I won't log a "Request Changes" review in case that mucks up process (especially as a non-Editor on the doc), but I do urge working through Resolving all of my comment threads except maybe the first one that I said was non-blocking. Also, thanks for updating the SHACL-SHACL shapes. |
@ajnelson-nist Thanks for reviewing the tests - I've gone back and put together simpler test cases to improve readability. I had used copilot to quickly put together the tests whilst the PR was in draft. They are now human crafted and checked.
This is now allowed; I was struggling to think of a case where it'd be used in practise - but I can think of a few including
Good point - now using SHACL lists; I'd forgotten they were defined in the doc. |
@TallTed I think I've addressed all of your requested changes - could you please update your review so this isn't blocked from merging. @ajnelson-nist Could I ask you to confirm your approval with the current state of this PR. |
rdf:type sh:NodeShape ; | ||
sh:memberShape [ sh:nodeKind sh:IRI ] ; | ||
sh:targetNode ( ex:Alice ex:Bob ) ; | ||
sh:targetNode () ; |
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 target isn't among the expected test results, but looks like it should trigger a violation.
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 a valid SHACL list (it is just rdf:nil
) so I expect this to pass.
I can see cases where you'd want to make sure the list does have have some elements - I've raised #414 to propose sh:minCollectionLength
and sh:maxCollectionLength
as constraints
sh:memberShape [ sh:nodeKind sh:IRI ] ; | ||
sh:targetNode ( ex:Alice ex:Bob ) ; | ||
sh:targetNode () ; | ||
sh:targetNode _:b1, _:b2, _:b3, _:b4, _:b5 ; |
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.
_:b3
isn't among the data.
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.
That is intended. It should raise a violation because a blank node with no properties is an invalid list.
Overall, the progress is good. My (soft-)blockers are:
Again, I won't push the "Request Changes" button in case that mucks up process. |
…d on Node Shapes and add appropriate tests
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
While looking over SHACL-SHACL updates today, I noticed there's a live use case for this PR within SHACL-SHACL. #160 / #395 expanded It's a bit debatably scope-creep, but I think ] ;
sh:property [
sh:path sh:class ;
- sh:nodeKind sh:IRI ; # class-nodeKind
+ sh:or (
+ [ sh:nodeKind sh:IRI ]
+ [ sh:memberShape [ sh:nodeKind sh:IRI ] ]
+ ) ; # class-nodeKind
] ;
sh:property [
sh:path sh:closed ; Though, thinking about the case (possibly pathologically?) of an IRI-identified ex:Foo
a rdfs:Class ;
.
ex:ExampleShape
sh:class ex:ExampleList ;
.
ex:ExampleList
rdf:first "foo" ;
rdf:rest (
ex:Foo
)
. All the above would also apply for I'm fine with this being broken out into a separate issue, not blocking this PR, examining 391 and 160 thrown against one another. |
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.
lgtm
Closes #391