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

GH-3992 add sparql parser default prefixes #3993

Merged

Conversation

ate47
Copy link
Contributor

@ate47 ate47 commented Jun 17, 2022

GitHub issue resolved: #3992

Briefly describe the changes proposed in this PR:

This PR add a constructor to the SPARQLParser class and a new method in the SPARQLParserFactory to add custom default prefixes to the parser.

// SPARQLParser
SPARQLParser(Set<Namespace> customDefaultPrefixes)

// SPARQLParserFactory
QueryParser getParser(Set<Namespace> customDefaultPrefixes);

Example

// create a set with the ex namespace 
Set<Namespace> customDefaultPrefixes = new HashSet<>();
customDefaultPrefixes.add(Values.namespace("ex", "http://example.org/"));

// create a parser with the default prefixes
SPARQLParser parser = new SPARQLParser(customDefaultPrefixes);

// we can use the default prefix without having to specify it
parser.parseQuery("SELECT ?s {?s ex:aaa ex:ooo}", null);

// we can redefine the prefix in the query if we want another one (this is not an error)
parser.parseQuery("PREFIX ex: <http://example2.org/> SELECT ?s {?s ex:aaa ex:ooo}", null);


// we cannot redefine a prefix in the same request (this is still an excepted error)
parser.parseQuery("PREFIX ex: <http://example.org/> PREFIX ex: <http://example2.org/> SELECT ?s {?s ex:aaa ex:ooo}", null);

PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

@barthanssens
Copy link
Contributor

Hi,

thanks for the PR, just wondering if e.g. a Set<Namespace> might be more elegant.
That would also allow for (re)using the predefined namespaces from org.eclipse.rdf4j.model.vocabulary....

@ate47
Copy link
Contributor Author

ate47 commented Jun 23, 2022

True! I wasn't aware of the existence of this package, I've replaced the map to a set parameter

Copy link

@EmptyStar EmptyStar left a comment

Choose a reason for hiding this comment

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

I'd like to make a case for using Collections.emptyMap() in favor of passing around null for the custom prefix map. Your code would be simpler and probably safer if you could always assume the prefix map to exist. I pointed out a few bits of code that could be made more elegant with an empty map.

@ate47
Copy link
Contributor Author

ate47 commented Jun 23, 2022

I'd like to make a case for using Collections.emptyMap() in favor of passing around null for the custom prefix map. Your code would be simpler and probably safer if you could always assume the prefix map to exist. I pointed out a few bits of code that could be made more elegant with an empty map.

Yes, I was a bit lazy to check if the PrefixDeclProcessor was used anywhere else to be able to remove use null instead, but after a check, it is not.

@ate47 ate47 force-pushed the GH-3992-sparql-default-prefixes branch from e8baebc to b7aa63f Compare June 23, 2022 09:19
@D063520
Copy link

D063520 commented Jun 28, 2022

Hi, is there something blocking? I'm in favour of this pull request : )

@barthanssens
Copy link
Contributor

Thanks for the enhancement !
Perhaps the only thing left to do would be squashing the commits (see also https://rdf4j.org/documentation/developer/squashing/ on how to do this, and https://rdf4j.org/documentation/developer/merge-strategy/ why this is a probably a good idea)

@ate47 ate47 force-pushed the GH-3992-sparql-default-prefixes branch from b7aa63f to fe916a3 Compare June 28, 2022 19:49
@ate47
Copy link
Contributor Author

ate47 commented Jun 28, 2022

Thanks for the enhancement ! Perhaps the only thing left to do would be squashing the commits (see also https://rdf4j.org/documentation/developer/squashing/ on how to do this, and https://rdf4j.org/documentation/developer/merge-strategy/ why this is a probably a good idea)

Done, I was thinking it was squashed enough.

@ate47 ate47 force-pushed the GH-3992-sparql-default-prefixes branch from fe916a3 to 30ee077 Compare June 29, 2022 07:18
Copy link
Contributor

@hmottestad hmottestad left a comment

Choose a reason for hiding this comment

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

I've added some comments. If you agree with my comments then ping me with @hmottestad when you're done and I'll merge this PR so it will be included in the upcoming 4.1.0 release.

@ate47 ate47 force-pushed the GH-3992-sparql-default-prefixes branch from 30ee077 to 664e355 Compare July 8, 2022 09:28
@hmottestad hmottestad merged commit 06a5b28 into eclipse-rdf4j:develop Jul 8, 2022
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.

5 participants