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

Feature RDF/JS #118

Merged
merged 34 commits into from
Apr 11, 2018
Merged

Feature RDF/JS #118

merged 34 commits into from
Apr 11, 2018

Conversation

rubensworks
Copy link
Member

@rubensworks rubensworks commented Oct 13, 2017

To do:

  • RDFJS data: N3Writer & N3StreamWriter
  • RDFJS data: N3Store
  • RDFJS data: N3Util

@rubensworks
Copy link
Member Author

N3Writer has been changed in a backwards-compatible way, the only cost of this is a simple typeof term === 'string' check on each term, so performance impact should be minimal.

I can still change it if you prefer a complete breaking change, in which case more invasive internal changes to the writer will need to be done.

@RubenVerborgh
Copy link
Member

Thanks @rubensworks, breaking changes are fine with me. v1.0.0 will not be backward compatible.

@rubensworks
Copy link
Member Author

@RubenVerborgh I removed backwards compatibility with the string-based interface of N3Writer. However, internally everything currently still works with the old string-based representation. I believe this will have better performance than using the RDF/JS datastructures for everything internally as well.

@rubensworks
Copy link
Member Author

rubensworks commented Oct 18, 2017

@RubenVerborgh Everything should be compatible with the RDF/JS interface now.

I had to make some design decisions, such as converting everything to a string-based representation internally, and the way N3Util.expandPrefixedName works.
Please let me know if you agree with these.

For reference, I've updated the TS typings, so you can see there what the (intended) types and signatures are.

@elf-pavlik
Copy link
Member

Have you tried running the test suite mentioned in rdfjs/data-model-spec#47 (comment) ?

@rubensworks
Copy link
Member Author

@elf-pavlik Thanks for the suggestion, I'll look into it soon.

@rubensworks
Copy link
Member Author

It looks like the test suite is built around DataFactory. This PR however does not provide a DataFactory implementation, so this can't be tested, unless it should?
@RubenVerborgh Thoughts?

@bergos
Copy link
Member

bergos commented Nov 5, 2017

@rubensworks the test suite moved to rdf-data-model. Old documentation is still valid.

@rubensworks
Copy link
Member Author

@RubenVerborgh N3Util now implements the DataFactory interface, as requested in #122.

The test suite from @bergos also succeeds, except for one test regarding default graphs, which I believe is a mistake in the tests (rdfjs-base/data-model#3)

@pietercolpaert
Copy link
Member

Is performance worse or similar with the new interface? @rubensworks

@RubenVerborgh
Copy link
Member

Slightly worse last time I tested, but acceptable.

@pietercolpaert
Copy link
Member

Would this PR make N3.js ready for a new major release?

@RubenVerborgh
Copy link
Member

RubenVerborgh commented Mar 19, 2018 via email

@RubenVerborgh RubenVerborgh force-pushed the feature-rdfjs branch 2 times, most recently from 78e50ae to f9b355e Compare April 9, 2018 13:56
@rdfjs rdfjs deleted a comment from coveralls Apr 10, 2018
@rdfjs rdfjs deleted a comment from coveralls Apr 10, 2018
It's up to webpack or browserify to do that.
@RubenVerborgh RubenVerborgh changed the base branch from master to develop April 11, 2018 22:39
@RubenVerborgh RubenVerborgh merged commit c6435ff into rdfjs:develop Apr 11, 2018
@RubenVerborgh RubenVerborgh deleted the feature-rdfjs branch April 11, 2018 22:41
@RubenVerborgh
Copy link
Member

Thanks for your help, @rubensworks. I've heavily modified the interfaces, so the TypeScript definitions will be obsolete. I've removed usage of the internal representation from most places, and many helper methods from Util are gone.

@rubensworks
Copy link
Member Author

Great! I will look into updating the TS typings soon.

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.

6 participants