Feature: callback and event for comments #423
Conversation
src/N3Parser.js
Outdated
| // ### `parse` parses the N3 input and emits each parsed quad through the callback | ||
| parse(input, quadCallback, prefixCallback) { | ||
| // ### `parse` parses the N3 input and emits each parsed quad through the callback. Optionally you can set a prefixCallback and a commentCallback | ||
| parse(input, quadCallback, prefixCallback, commentCallback) { |
There was a problem hiding this comment.
We're getting too many arguments here; let's support:
parse(input, onQuad)parse(input, onQuad, onPrefix)parse(input, { onQuad, onPrefix, onComment })
the first two of which are for legacy compatibility.
There was a problem hiding this comment.
Ok, will amend my PR accordingly
|
Note that the slightly weird suggestions here are because this parser is written quite close to the V8 engine; some functions execute 200,000+ times per second, so every |
|
All comments have been processed. At your own convenience, can you give this PR another review? |
There was a problem hiding this comment.
The CI is not kicking in, weird.
Mostly okay, but we'll need the result from the performance tests (they're in the repo). What is the difference? The streaming parser has unconditional comments currently, so there would be a penalty (but the perf tests aren't looking for them unfortunately).
|
I made emitting comments in the streamparser optional and not the default now. |
There was a problem hiding this comment.
CI can be seen at https://github.com/pietercolpaert/N3.js/tree/feature-comments; please fix coverage issues.
Beyond that LGTM. When done please squash merge with commit message starting with feat: so semantic release runs correctly.
Co-authored-by: Ruben Verborgh <[email protected]>
|
Thanks @jeswr and @RubenVerborgh for the extremely fast and thorough review process! I’ve added some tests just now that fix the coverage. It appeared locally the coverage didn’t trigger due to other experimental tests I was running that did cover the line. Squashing is something @RubenVerborgh will do? Should I also add documentation in the README.md on how to enable comments? |
Yes, that would be a good idea! Ruben and I both have permission to merge so can do that once the new documentation is added. |
|
Added the documentation in the README.md, finalizing this PR! |
|
Thanks for your work @pietercolpaert ! |
|
🎉 This PR is included in version 1.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Limitation: timing of |
For my project I need access to the comments in a turtle (etc.) file.
This pull request: