-
Notifications
You must be signed in to change notification settings - Fork 10
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
merge sparql endpoints #127
Conversation
update sparql functions update tests add otherAnnotationProps to annotations remove startup graph posting
…ts in SPARQL queries, so exclude from the query if there are no terms.
Remove sequence path for sh:color Upudate env template
Could I please get a fairly comprehensive review of this as the changes are significant and breaking. In particular testing with the frontend. I have tested visually with the FE. |
I've just noticed /object is not working - will fix later today. This shouldn't block others reviewing the changes. |
Remove relative properties functionality - path/sequence path functionality has been extended to listing queries
…/narrower is a bit blunt. It includes a lot of unintended annotations as it applies at the profile level. The only other option would be to interpret a combination of sequence and inverse path as concepts are skos:inScheme the focus node (inverse path), and the additional properties we want on them (broader, narrower) are two steps away from the focus node. This is trivial in SPARQL but probably requires more careful design in 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.
This was great fun to review: I was worried about the shear number of files but I got through them quickly since it's mostly code removal and simplification! Also, I'm thrilled to see your really neat functions and bonus documentation @recalcitrantsupplant! Great PR.
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.
Phew, finished! Looks good, just left some minor comments in there. I'll run it with prez-ui and test it tomorrow. Should be good to go shortly after that.
Requested change in https://github.com/RDFLib/prez/pull/127/files/2fcd93ab6a7daf90e9bf32f4b02386c0a10750b9#r1236251610 will fix #127. |
This line needs to be removed. Line 81 in 2fcd93a
|
I think the last thing is to remove Lines 70 to 80 in f90b743
|
Once you've made that change in the last comment, I'll do one last test and distill the changes into dot points here so we can paste it into the release notes. |
Thanks - removed now |
Fixes
Changes
Added
Breaking
@recalcitrantsupplant do you agree with this list? |
One last thing, can you fix the formatting for the documentation section on annotation properties please? See https://github.com/RDFLib/prez/blob/798be47cdf185cc10d2078db1de40f7fb2e1831f/README-Dev.md#annotation-properties. |
Cheers, I've edited your comment to include info on "annotated SPARQL queries" under Added, and fixed the markdown table. |
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.
Looking good now. Thanks @recalcitrantsupplant for addressing my comments.
update sparql functions
update tests
add otherAnnotationProps to annotations
remove startup graph posting