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

Skip n3 parser for jsonld #358

Merged
merged 3 commits into from
Oct 22, 2019

Conversation

rescribet
Copy link
Collaborator

@rescribet rescribet commented Oct 2, 2019

This removes the n3 library step when parsing json-ld.

I made two prior commit in order to make it possible for the implementation to use the proper rdfjs task force data factory methods on formula/indexed formula/store.

@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch from d22d57d to 069bae3 Compare October 2, 2019 15:31
@dmitrizagidulin
Copy link
Contributor

@Fletcher91 Good work. I don't have opinions on whether or not we should adopt the ?? language feature.
But I'm glad you're working on this.

@rescribet
Copy link
Collaborator Author

rescribet commented Oct 7, 2019

TODO: Add conditional for collections

Fixed

src/jsonldparser.js Show resolved Hide resolved
src/jsonldparser.js Show resolved Hide resolved
@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch 2 times, most recently from 35eab77 to c2683a6 Compare October 15, 2019 10:06
@rescribet
Copy link
Collaborator Author

rescribet commented Oct 15, 2019

I've added parsing to collections by default, which can be disabled for json-ld, n3, and turtle by setting rdfFactory.supports['COLLECTIONS'] = false, the capitals are used with a nice enum in mind.

This also relates to #137 in that formula/store now accept the datafactory to be overridden by the end-user (as suggested to me by @RubenVerborgh), though all defaults are still there and no user-change should be required to keep current behaviour. Note the added supports property on the datafactory as a clean way for low-level code to determine which functionality is present on the factory (e.g. the rdflib specific collection terms) and allows the end-user to disable it as well (#169).

@rescribet rescribet requested a review from timbl October 15, 2019 10:18
@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch 4 times, most recently from 91df63e to a0be184 Compare October 17, 2019 13:43
Copy link
Member

@timbl timbl 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 as far as I can see -- just looking at the clllections part

@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch from a0be184 to ed6b494 Compare October 18, 2019 05:55
@rescribet
Copy link
Collaborator Author

rescribet commented Oct 18, 2019

Fixed and rebased a typo I noticed.

Edit: Testing this branch against an existing project, coming in with some more fixes

Changes work properly now

@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch 2 times, most recently from 9ea7462 to 1175c96 Compare October 18, 2019 14:14
Fletcher91 added 3 commits October 18, 2019 17:15
The current implementation contains some methods which are like the spec
DataFactory interface but have small distinctions. E.g. `namedNode` is
called `sym`, `literal` accepts three rather than two arguments. This
commit applies the canonical methods of the rdfjs tf DataFactory
interface to the store. End-users can also overwrite the used factory
via a constructor argument.

This also replaces the use of sameTerm for the spec version equals, which
was already present in the code (sameTerm returned the result of equals).
Toggling the collections flag on the datafactory supports object will
disable the generation of collection objects in-memory. It is still
enabled by default for rdflib.
@rescribet rescribet force-pushed the skip-n3-parser-for-jsonld branch from 1175c96 to 14c4ae5 Compare October 18, 2019 15:16
@dmitrizagidulin
Copy link
Contributor

+1 from me.

@Fletcher91 - I don't suppose you know how to fix #364 by any chance? I tried it out with your branch from this PR, and it still gives that error. This is not to say this PR shouldn't be merged - totally should, I'm just curious if you know a fix.

@rescribet
Copy link
Collaborator Author

@dmitrizagidulin This PR focuses on parsing, not serialization, so that's not a surprise to me. Looking at serialize.js it also goes through the n3 code, but that's no wonder since the jsonld package has a very peculiar API for converting from/to statements (the docs call a serialized n-quads formatted string 'rdf').

They also support js objects in toRDF, so circumventing the N3 library for serialization is also possible.

Note that that functionality can't be used for parsing since they don't support collections as termtype, otherwise this code (jsonldparser.js) could be a lot simpler

@dmitrizagidulin
Copy link
Contributor

@Fletcher91 Thanks, np.

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 21, 2019

@Fletcher91, could you write a small note on what's changed that users of rdflib might want to be aware of? And I presume this does not change the public API?

@rescribet
Copy link
Collaborator Author

@Vinnl This shouldn't break the current API (only expand upon).

The main thing is that the store has an additional property rdfFactory which is the datafactory which is used by all the components, I've added a supports property to the factory to indicate to re-users (e.g. the serializer) what (custom) features are supported/enabled, like the rdflib specific collections.

In addition, util.js contains some new functions (isNamedNode, isBlankNode, isStatement {rename to isQuad?}) which check if something is the correct type, these seem superfluous but will be really useful when using typescript (type assertion).

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 21, 2019

Thanks, I'll merge and release tomorrow, given that it's been approved.

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 22, 2019

Correction: I'm just merging for now, not releasing. Our CI setup isn't extensive enough to give me confidence that this won't break anything, and we're also releasing and deploying new versions of all the Data Browser libraries (mashlib, solid-panes, solid-ui) today, so I'd like to only include those changes there for now. Let me know if that's problematic for anyone.

@Vinnl Vinnl merged commit 6cf0a2c into linkeddata:master Oct 22, 2019
@rescribet
Copy link
Collaborator Author

@Vinnl Could you publish a prerelease version? It'll be easier to test in downstream projects

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 22, 2019

@Fletcher91 Sure, [email protected].

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.

6 participants