-
Notifications
You must be signed in to change notification settings - Fork 4
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
Improve SKOS Concept parsing to allow use of intermediate SKOS Concept #10
Improve SKOS Concept parsing to allow use of intermediate SKOS Concept #10
Conversation
Add frozen_string_literal: true, it improves ruby performance by making string immutable, thus reducing object allocation
It describe the expected API for product types and facets
It contains method needed by both SKOSInstance and SKOSConcept
This is used to by the ruby codegen to add specific code to the SKOSConcept class
This so that SKOSParser can use SKOSConcept instead of SKOSInstance when needed
I don't think the first API make total sense, fix spec to match current parsing implementation
It is easily extandable if we need more Concept Scheme to be parsed using SkosConcept, you just need to add the name to CONCEPT_SCHEME
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.
Great! This solves so many scenario.
My suggestions below are just cosmetic, really.
src/org/datafoodconsortium/connector/codegen/ruby/static/spec/parse_with_skos_concept_spec.rb
Outdated
Show resolved
Hide resolved
src/org/datafoodconsortium/connector/codegen/ruby/static/spec/parse_with_skos_concept_spec.rb
Outdated
Show resolved
Hide resolved
if element["http://www.w3.org/2004/02/skos/core#narrower"] | ||
element["http://www.w3.org/2004/02/skos/core#narrower"].each do |narrower| | ||
@narrower.push(narrower["@id"]) | ||
end | ||
end |
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.
Would this work?
if element["http://www.w3.org/2004/02/skos/core#narrower"] | |
element["http://www.w3.org/2004/02/skos/core#narrower"].each do |narrower| | |
@narrower.push(narrower["@id"]) | |
end | |
end | |
element["http://www.w3.org/2004/02/skos/core#narrower"]&.each do |narrower| | |
@narrower.push(narrower["@id"]) | |
end |
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.
It does, but I find it harder to read.
...foodconsortium/connector/codegen/ruby/static/lib/datafoodconsortium/connector/skos_parser.rb
Outdated
Show resolved
Hide resolved
src/org/datafoodconsortium/connector/codegen/ruby/static/spec/parse_with_skos_concept_spec.rb
Outdated
Show resolved
Hide resolved
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.
Excellent! I'm looking forward to the merge.
src/org/datafoodconsortium/connector/codegen/ruby/static/CHANGELOG.md
Outdated
Show resolved
Hide resolved
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.
Thank you @rioug.
I've made some recommendations, could you handle them?
Could you also update the general changelog (at the root of the project)?
I think one day the ruby connector should use a JSON-LD parser instead of our custom made one. The Ruby-RDF project seems to provide some great libs like json-ld. We should consider to use this some day. @mkllnk There is also a lib for LDP, it may be useful.
Like you said @rioug it would be nice to be able to navigate the tree like narrowers and broaders. It was the plan but I was not sure it would be used and at the beginning the connector was not supposed to provide a SKOS parser. But I think it is useful for everyone to have it.
@rioug I don't think narrowers was used in the existing behaviour of SKOSConcept. I think it is not populated by the parser.
src/org/datafoodconsortium/connector/codegen/ruby/classifier.mtl
Outdated
Show resolved
Hide resolved
src/org/datafoodconsortium/connector/codegen/ruby/operation.mtl
Outdated
Show resolved
Hide resolved
6baedc7
to
905c8b8
Compare
@lecoqlibre all done!
I am curious of what you are doing in the other language, looks like you are just querying the parse JSON for php, is that right ? |
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.
Thanks @rioug!
@lecoqlibre all done!
Like you said @rioug it would be nice to be able to navigate the tree like narrowers and broaders. It was the plan but I was not sure it would be used and at the beginning the connector was not supposed to provide a SKOS parser. But I think it is useful for everyone to have it.
I am curious of what you are doing in the other language, looks like you are just querying the parse JSON for php, is that right ?
The PHP version has an import method that save all the loaded RDF subjects (objects) into a store. We can then get them using a retrieve method (fetch). The intermediate SKOS concepts should be available in the store.
On the client side, in JavaScript/TypeScript we can use LDFlex which allows to access linked data using standard JavaScript dot property syntax. I use it in my different Solid projects, it's very nice and it's based on the incredible Comunica library (make SPARQL queries overs HTTP).
This PR fixes the following issues :
Additional background : datafoodconsortium/connector-ruby#13 (comment)
It allows the ruby connector to use intermediate SKOS concept for Product Types and Facets ie:
connector.FACETS.TERRITORIALORIGIN.EUROPE.FRANCE
connector.PRODUCT_TYPES.DRINK.SOFT_DRINK
It didn't make sense to use intermediate for Measures so I kept the original behaviour (it can be easily fixed if needed)
I am not familiar with the design, but I don't think it makes sense for
DataFoodConsortium::Connector::SKOSConcept
to be a subclass ofDataFoodConsortium::Connector::SKOSInstance
, so I used a helper module to share method needed by both class.This also populates
narrowers
andbroaders
forSKOSConcept
so it's easier to navigate the tree. Ideally I would have like them to return a list of methods so could do something like below without additional steps :I didn't want to alter the existing behaviour of
DataFoodConsortium::Connector::SKOSConcept#narrowers
, that said the original values would still be available viaskos:narrower
semantic property. Any thought ?This also populates
prefLabels
forDataFoodConsortium::Connector::SKOSConcept
, I used an hash with locale has keys for ease of use ie :