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

Added support for SPARQL language #2033

Merged
merged 14 commits into from
Sep 5, 2019
Merged

Added support for SPARQL language #2033

merged 14 commits into from
Sep 5, 2019

Conversation

thomasdegroot18
Copy link
Contributor

This is the support for SPARQL based on the W3C SPARQL specifications
https://www.w3.org/TR/sparql11-query/

All of the queries from the specifications are included in the tests and as examples.

@RunDevelopment
Copy link
Member

Thank you for this PR @thomasdegroot18!

Before I'll review this, a few minor things:

  1. Please move sparql below soy. The list is sorted alphabetically.
  2. We use tabs for indentation. This is true for all files: .html, .js, .test.
  3. Please remove the g flags from all patterns. Prism will add a g if it has to.
  4. Travis failed because you forgot to rebuild Prism. Run npx gulp to rebuild Prism.

I'll properly review your PR soon.

@thomasdegroot18
Copy link
Contributor Author

Thanks for the quick comments.

I've updated the location, changed everything in tabs now, removed the g flags and reran npx gulp.
Thanks for the update.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

I took another quick look and noticed that you probably used the Turtle language as a base.
Instead of copying the patterns, you can use Prism.languages,extend('turtle', { ... }) to do the copying for you. This will create a (deep) copy of Turtle and allows to change the patterns of your copy. In combination with Prism.languages.insertBefore, you should be able to write your language definition with less redundancy.
Note: You have to require turtle in components.json for this to work.

This is just an idea of mine as someone who has never worked with SPARQL before and hasn't read the spec yet, but:
Couldn't we just do this:

Prism.languages.sparql = Prism.languages.extend('turtle', {
    'keyword' [ /* the 3 new keyword patterns */ ],
    // 'operator': /the pattern/
});

Prism.languages.insertBefore('sparql', 'punctuation', {
    'variable': /[?$]\w+/
});

Turtle and Sparql just look so similar. Please tell me if I got anything wrong. Again, I don't know either language.

Also:

  1. We only use single-quoted strings.
  2. Please wrap token names in single quotes. This doesn't change anything semantically but it makes the language definitions easier to read in IDEs (different colors). Example:
    'string': {
        pattern: /"(?:[^\\"\r\n\t\b\f])*"|'(?:[^\\'\r\n\t\b\f])*'/,
        greedy: true
    },
  3. A lot of the comments are kinda repetitive and describe the behavior Prism more than your patterns, so please remove some.

components.json Outdated Show resolved Hide resolved
components/prism-sparql.js Outdated Show resolved Hide resolved
@thomasdegroot18
Copy link
Contributor Author

After checking it, the SPARQL spec and the turtle spec are a bit to diverged to extend one onto the other. Due to most of the changes to both of the specs, both are similar but differ quite a lot in the basics.

I've updated the code with the comments about the quotes and the array.

@RunDevelopment
Copy link
Member

While it's true that SPARQL and Turtle don't share a lot when it comes to semantics, their syntax is quite similar on purpose:

Most forms of SPARQL query contain a set of triple patterns called a basic graph pattern. Triple patterns are like RDF triples except that each of the subject, predicate and object may be a variable. (source)

Turtle provides levels of compatibility with the N-Triples [N-TRIPLES] format as well as the triple pattern syntax of the SPARQL W3C Recommendation. (source)

Also, extending in Prism doesn't have any strict definitions or restrictions. It's just a way to share common patterns and structures between language definitions. I suggested extending from Turtle because both language definitions seem very similar and because extending generally reduces redundancy across language definitions and the overall bundle size of Prism.

The only differences between Turtle and SPARQL that I see are the actual syntactical differences (new (e.g. variable) and drastically changed (e.g. keyword) patterns) and small differences where you abided to spec more closely than I and the author of #2012 did (e.g. iri) and where you didn't quite adhere to the spec (e.g. string).

I'll make an update to make Turtle adhere to its spec more closely and to fix a few small mistakes I found.

@thomasdegroot18
Copy link
Contributor Author

Thanks for the update, I will update the code accordingly and update the pull request.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor things.

components.json Show resolved Hide resolved
components.json Outdated Show resolved Hide resolved
components/prism-sparql.js Outdated Show resolved Hide resolved
components/prism-sparql.js Outdated Show resolved Hide resolved
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

2 more things and then we can merge this!

tests/languages/sparql/keyword_feature.test Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@RunDevelopment RunDevelopment merged commit c42f877 into PrismJS:master Sep 5, 2019
@RunDevelopment
Copy link
Member

Thank you for contributing!

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

Successfully merging this pull request may close these issues.

None yet

2 participants