-
Notifications
You must be signed in to change notification settings - Fork 38
Issue 415: Use sh:memberShape
in SHACL SHACL
#417
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
sh:memberShape
in SHACL SHACLsh:memberShape
in SHACL SHACL
44c396a
to
eaf5d95
Compare
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 found one minor thing to fix, and highlighted a spec. incongruity. I'd like feedback on what to do about the incongruity before I approve.
sh:node shsh:ListShape ; # xone-node | ||
sh:memberShape shsh:ShapeShape ; # xone-memberShape |
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.
Interesting. The test has caught up with an uncaught corner case of the spec.
Section 4.6.4 of the 1.0 spec., consistent with today's draft state, reads:
sh:xone
specifies the condition that each value node conforms to exactly one of the provided shapes.
However, the remainder of that section doesn't talk about whether sh:xone
can or can't be empty. This shape in SHACL-SHACL is consistent with that lack of detail, but the validation for a user with any targeted nodes would always fail, because there isn't exactly one match.
Thinking about it a bit, this seems like it would be a backwards-incompatible change to add a sh:minListLength 1
here that fails a SHACL-SHACL run, because the sh:xone
subject could be defined with no targets and sneak in as valid against SHACL-SHACL of SHACL 1.0.
Thoughts on whether we try fixing this now, or adding a warning-severity shape in SHACL-SHACL?
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 would suggest adding text to the 1.2 spec requiring at least one shape - and reflecting that here with a minListLength of 1.
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.
Drafting an explanatory comment, I now think this is better left delegated to a follow-on Issue. Please leave this thread unresolved for reference.
2f88f1d
to
2ad5655
Compare
2ad5655
to
8ffae40
Compare
…:xone This patch revises part of the implementation in PR 417. * `in-minListLength` was not added to the Core document, but it was referenced in SHACL-SHACL. Verbiage is added to Core in line with the text drafted for Issue 429. * `xone-memberShape` was added as a reference in SHACL-SHACL, but doesn't exist in the Core document. `xone-members-node` is the anchor in the Core document. `xone-members-node` is now used. * The `PropertyShape` for `sh:in` set a minimum required length of 1, which after discussion in Issue 429 appears to be a backward- incompatible change. A shape paralleling the original suggestion for `sh:xone` is added to house that new restriction at the same warning- level severity. An implementation style point: The property shapes housing `sh:minListLength` are given IRIs after sketching a test (`.../in-003.ttl`) and assuming it would be more desirable to have an IRI-identified shape in the results graph, rather than a blank node. A testing style point: Rather than running SHACL-SHACL over the whole of a test, shapes from SHACL-SHACL are copied into some focused tests (`.../{in,xone}-003.ttl`). References: * #429 * #417 Signed-off-by: Alex Nelson <[email protected]>
Closes #415