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.

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

Issue 669 2 #671

Merged
merged 9 commits into from
Jan 8, 2018
Merged

Issue 669 2 #671

merged 9 commits into from
Jan 8, 2018

Conversation

tfrancart
Copy link
Contributor

(depends on other PR "issue-669-1" to work properly)
I realize this may not be the correct way to proceed, since the commits already in issue-669-1 are also in this branch. let me know.

@osma
Copy link
Member

osma commented Dec 4, 2017

What's in this PR that's not in #670?
I think #670 should be sorted out and merged first.

Note that the Travis build failed for this PR but not for #670.

@tfrancart
Copy link
Contributor Author

Thanks.
#670 only contains modifications in GenericSparql. This one modifies also Concept.php based on updates in GenericSparql.
Using the getLabel method in GenericSparql imposes a new constraint : the declared properties must have an rdf:type declared (because the SPARQL query imposes that). An extra rdf:type needs to be added in the test cases data. That's why some tests fail.
I will propose a fix on the test data.
I don't think this is problematic; do you have an opinion on this ?

@tfrancart
Copy link
Contributor Author

Hi @osma if you had time to review and merge this in the next couple of weeks that would be great. We are targeting the deployment of Skosmos to publish legal vocabularies in early january, and having this improvement in the master branch would help.
Let me know.
Thanks

@kouralex kouralex requested a review from henriyli December 15, 2017 13:28
@osma
Copy link
Member

osma commented Dec 19, 2017

We tested this with @henriyli and found out this imposes further constraints in addition to the rdf:type requirement you mention: the property definisions must be available in the default SPARQL endpoint, and union default graph mode needs to be enabled. If there is a property definition within the vocabulary, it won't be used if it's not available through the default graph of the default endpoint. So far Skosmos has not required a union default graph, and the current InstallTutorial section on Fuseki configuration leaves it commented out i.e. disabled.

I wonder if it would be easy to also check the property definitions from within the current vocabulary itself, as is the current behavior?

@tfrancart
Copy link
Contributor Author

Thanks for reviewing. I have modified the method so that it first queries the current vocabulary for property labels/superprops, before defaulting to the default SPARQL endpoint.
Note that for properties with no label at all anywhere, this will make another additionnal unnecessary query.

@osma
Copy link
Member

osma commented Dec 22, 2017

Thanks, but that's probably not the best way to do it. The property definitions (including label and rdfs:subPropertyOf) are already included in the CONSTRUCT query result for the current concept, so there's no need to perform yet another SPARQL query. See the original code around line 355 in Concept.php that you deleted already in the first version of this PR. There is no need to perform another SPARQL query to get that information if it's already in the graph we have. Only if it's not there does it make sense to query the default SPARQL endpoint.

@tfrancart
Copy link
Contributor Author

OK done.

@henriyli henriyli merged commit 43d19f9 into NatLibFi:master Jan 8, 2018
@henriyli
Copy link
Collaborator

henriyli commented Jan 8, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants