-
Notifications
You must be signed in to change notification settings - Fork 35
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
Prevent duplicate prefixes #163
base: master
Are you sure you want to change the base?
Prevent duplicate prefixes #163
Conversation
Instead of parsing the query manually, I would recommend plugging into the actual SPARQL parser. I don't remember if the jquery widget here will actually call the parser here directly. |
Since it doesn't look like Jquery calls the parser, I'll have a look at doing it further up. Comunica seems like the logical place to do duplicate handling. For this case, using the last-defined prefix in the case of a duplicate should work. |
src/ldf-client-ui.js
Outdated
var prefixesString = ''; | ||
if (this.options.queryFormat === 'sparql') { | ||
for (var prefix in this.options.prefixes) | ||
prefixesString += 'PREFIX ' + prefix + ': <' + this.options.prefixes[prefix] + '>\n'; | ||
} | ||
let query = prefixesString + queryWithoutPrefixes; | ||
let query = prefixesString + this.$queryTextsIndexed[this.options.queryFormat].val(); |
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.
Could you check if it's possible to pass these prefixes directly to the SparqlParser options?
I think there was an option for that.
If that's possible, we don't need the prefixesString
anymore.
src/ldf-client-ui.js
Outdated
let query = prefixesString + this.$queryTextsIndexed[this.options.queryFormat].val(); | ||
|
||
// Remove duplicate prefixes | ||
const parsedQuery = new SparqlParser({ sparqlStar: true }).parse(query); |
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 can only be done if if (this.options.queryFormat === 'sparql') {
(because queries can also be defined as GraphQL)
prefixesString += 'PREFIX ' + prefix + ': <' + this.options.prefixes[prefix] + '>\n'; | ||
// Add pre-defined prefixes to query and remove duplicates | ||
const parsedQuery = new SparqlParser({ prefixes:this.options.prefixes, sparqlStar: true }).parse(query); | ||
query = new SparqlGenerator({}).stringify(parsedQuery); |
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 looks perfect!
a50c364
to
5432f57
Compare
Fixing Issue #149.
Some questions:
Should I also account for "PREFIX" not being written all caps in the query?
Can I assume that PREFIX lines start with PREFIX or with only whitespace before?
Can I assume that each PREFIX declaration is on a seperate line starting with PREFIX? Or could there be multiple?
Can prefixes be declared deeper in the query or only at the top?