Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#288: Added sh:SelectExpression, with test cases and TTL changes #297

Merged
merged 4 commits into from
Mar 7, 2025
Merged

#288: Added sh:SelectExpression, with test cases and TTL changes #297

merged 4 commits into from
Mar 7, 2025

Conversation

HolgerKnublauch
Copy link
Contributor

No description provided.

@ajnelson-nist
#222, #288: Update SHACL-SHACL for general NodeExpression and SelectE…
8632a3e 
…xpression

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist
Copy link
Contributor

@HolgerKnublauch - I added an update to SHACL-SHACL. It seems one update should have gone in for #274 : If sh:targetNode can now allow blank nodes in their object position, then it seems there's currently no sh:PropertyShape necessary because there's no constraint to specify. Does this need to be broken out into a separate PR, possibly also to address any updates to targetNode-nodeKind text?

@ajnelson-nist ajnelson-nist linked an issue Mar 4, 2025 that may be closed by this pull request
Add sh:SelectExpression to SHACL-SPARQL #288
Closed
@ajnelson-nist ajnelson-nist mentioned this pull request Mar 4, 2025
Closed
bergos
bergos approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@bergos bergos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as mentioned in #301, we may need to revisit the sh:path property. But that should be done in a separate PR.

@ajnelson-nist
Copy link
Contributor

Oh dear. For awareness, there is a comment thread tied to just the last commit, here, documenting that I'm going to undo the SHACL-SHACL changes and move them to another PR. It's not displaying in this PR page. Sorry @bergos , you'll need to re-review.

@ajnelson-nist
#222, #288: Undo update SHACL-SHACL for general NodeExpression and Se…
2d7035e 
…lectExpression

This patch undoes commit 8632a3e.  This was done manually instead of with `git revert`.

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist ajnelson-nist mentioned this pull request Mar 4, 2025
Draft
@ajnelson-nist
Copy link
Contributor

Ok, the updates to SHACL-SHACL I had here are now moved out-of-band to #302 .

TallTed
TallTed previously requested changes Mar 6, 2025
View reviewed changes
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@HolgerKnublauch @TallTed
Update shacl12-vocabularies/shacl.ttl
d606a40 
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@HolgerKnublauch HolgerKnublauch requested a review from TallTed March 6, 2025 01:31
@HolgerKnublauch
Copy link
Contributor Author

@TallTed could you kindly recheck whether you can unblock this PR? Thanks.

@HolgerKnublauch HolgerKnublauch mentioned this pull request Mar 6, 2025
Merged
afs
afs approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@afs afs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for a FPWD.
I can't check the test case (no code).
There details recorded elsewhere for the example using sh:path.

@HolgerKnublauch HolgerKnublauch requested review from TallTed and removed request for TallTed March 7, 2025 06:59
@HolgerKnublauch
Copy link
Contributor Author

@TallTed this PR is still blocked because you have requested changes. As I have long applied your suggestion, I believe you still need to change the status of your review away from Request Changes.

(I don't think simple change requests such as typo fixes should ever block progress)

@caribouW3
Copy link
Member

@TallTed this PR is still blocked because you have requested changes. As I have long applied your suggestion, I believe you still need to change the status of your review away from Request Changes.

(I don't think simple change requests such as typo fixes should ever block progress)

Since that request has been already accepted, I'll dismiss the review comment to unlock merging.

@caribouW3 caribouW3 dismissed TallTed’s stale review March 7, 2025 09:48

Proposed changed has been accepted already.

@HolgerKnublauch
Copy link
Contributor Author

Thanks a lot, @caribouW3. I am going to merge this PR into the main branch now because it has sufficient approvals and has been widely agreed to be done by the group when #222 was accepted. The Core spec even contains a reference to this already, so it's best to get this consistent now. Also, there are follow-up branches based on this already piling up.

@HolgerKnublauch HolgerKnublauch merged commit a62647f into gh-pages Mar 7, 2025
1 check passed
@HolgerKnublauch HolgerKnublauch deleted the issue-288-SelectExpression branch March 7, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afs afs afs approved these changes

@bergos bergos bergos approved these changes

@ashleysommer ashleysommer Awaiting requested review from ashleysommer

@TallTed TallTed Awaiting requested review from TallTed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sh:SelectExpression to SHACL-SPARQL
7 participants
@HolgerKnublauch @ajnelson-nist @caribouW3 @afs @tpluscode @bergos @TallTed