Migrates to json-event-parser #43

Tpt · 2022-08-31T12:13:44Z

Makes bindings parsing work really in streaming

Tpt · 2022-08-31T12:15:30Z

The CI should work when json-event-parser is released

coveralls · 2022-09-05T12:37:16Z

Pull Request Test Coverage Report for Build 2993246318

0 of 89 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 2993136441:	0.0%
Covered Lines:	126
Relevant Lines:	126

💛 - Coveralls

coveralls · 2022-09-05T12:37:17Z

Pull Request Test Coverage Report for Build 3180161106

28 of 28 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 3000779580:	0.0%
Covered Lines:	66
Relevant Lines:	66

💛 - Coveralls

package.json

rubensworks · 2022-09-05T12:39:27Z

@Tpt Is this ready for review? Or just for initial testing of the new lib, and only to be used upon first stable release?

Tpt · 2022-09-05T12:40:28Z

@Tpt Is this ready for review? Or just for initial testing of the new lib, and only to be used upon first stable release?

Yes, it should be ready for review.

rubensworks

Nice work!
Just some general questions on where everything should be located.

@surilindur could you test this version on your end to see if it fixes comunica/comunica#1052 and #42 (in the jQuery widget without polyfills)?

rubensworks · 2022-09-05T13:47:37Z

test/SparqlJsonParser-test.ts

@@ -96,6 +96,17 @@ describe('SparqlJsonParser', () => {
 `)))).toEqual([]);
    });

+    it('should convert a slightly invalid empty SPARQL JSON response (common PHP error)', async () => {


Why is this slightly invalid? (looks valid to me at first glance)

Because the "bindings" key value should be an array and not an object.

lib/JsonStreamQueryTransformer.ts

surilindur · 2022-10-03T17:31:16Z

I am sorry, I completely forgot this!

With the changes here, I could no longer reproduce comunica/comunica#1052. The Buffer polyfill could also be removed (for issue #42) by bumping the version of readable-stream in json-event-parser to ^4.2.0.

Tpt · 2022-10-03T19:10:34Z

@surilindur Amazing news! Thank you!

@rubensworks Do you want to release a new beta of json-event-parser to allow me to update this PR with the new JsonStreamQueryTransformer implementation?

rubensworks · 2022-10-04T06:05:55Z

Done! Released v1.0.0-beta.2.

Makes bindings parsing work really in streaming

Tpt · 2022-10-04T07:21:04Z

@rubensworks Thank you! I have updated the PR to use the beta 2.

rubensworks

Nice! This one should be ready to merge then, once json-event-parser is out of beta.

rubensworks · 2022-10-04T07:32:17Z

package.json

@@ -32,6 +32,7 @@
  "devDependencies": {
    "@types/jest": "^28.0.0",
    "@types/minimist": "^1.2.0",
+    "@types/readable-stream": "^2.3.14",


I guess this one should be added as non-dev dep to json-event-parser.

Not sure it's required. json-event-parser works from plain JS projects without needing @types/readable-stream. What do you think about it?

The convention in TS is that typings packages are included as non-dev deps, so that downstream users don't have to include all of these typing packages manually (which can incur in a lot of overhead for many transitive deps). And since typings packages are usually very small, this is not a problem for non-TS projects.

Thank you! Done: comunica/json-event-parser.js#2

LuisAverhoff · 2022-10-07T16:00:11Z

Is there anything left to be done to get the json-event-parser package out of beta?

Tpt · 2022-10-10T15:17:57Z

@LuisAverhoff Not much. I believe it's good to go as a MVP. At worst we can easily push a 2.0.

rubensworks · 2022-10-11T04:41:07Z

I believe it's good to go as a MVP. At worst we can easily push a 2.0.

Do ping me when you think it's good to go :-)

Tpt · 2022-10-11T07:48:54Z

I believe it's good to go as a MVP. At worst we can easily push a 2.0.

Do ping me when you think it's good to go :-)

I believe that when comunica/json-event-parser.js#2 is merged, the codebase should be ready to be released.

rubensworks · 2022-10-11T08:00:35Z

@Tpt Great!

Before we release, could you look into configuring coveralls in GH Actions, and a badge in the readme (see some of my other repos as example)?

I also see coverage is not 100% yet. Would it be feasible to bump this to 100%?

Does the current implementation cover all of the requirements of rubensworks/jsonld-streaming-parser.js#76? (it's ok if this is not the case if this is planned for a next version, we just have to keep backwards-compat in mind then)

Tpt · 2022-10-12T08:19:20Z

Before we release, could you look into configuring coveralls in GH Actions, and a badge in the readme (see some of my other repos as example)?
I also see coverage is not 100% yet. Would it be feasible to bump this to 100%?

Sure, working on it. I'm also going to make the parser validates the JSON a bit more carefully to avoid bad surprises (very invalid numbers...).

Does the current implementation cover all of the requirements of rubensworks/jsonld-streaming-parser.js#76? (it's ok if this is not the case if this is planned for a next version, we just have to keep backwards-compat in mind then)

I think so. The basic parser seems to be working. I might just have to write some utilities on top of it but they are likely to be layers on top of the JsonEventParser instead of modifications of it.

rubensworks · 2022-10-12T08:24:41Z

I think so. The basic parser seems to be working. I might just have to write some utilities on top of it but they are likely to be layers on top of the JsonEventParser instead of modifications of it.

Nice :-)

Would be good to have some test in the JSON-LD parser repo to validate this.
E.g., the creation of an infinite JSON-LD stream, piping it into the JSON-LD streaming parser (in streaming mode), and inspecting that memory usage should remain constant (until the script is manually terminated).

Tpt mentioned this pull request Aug 31, 2022

Buffer is not defined #42

Closed

rubensworks force-pushed the master branch from a403a26 to 2a49b9d Compare September 5, 2022 12:19

Tpt force-pushed the stream branch from 208db77 to ffcc306 Compare September 5, 2022 12:35

rubensworks reviewed Sep 5, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Tpt force-pushed the stream branch from ffcc306 to 2252aef Compare September 5, 2022 12:39

rubensworks requested changes Sep 5, 2022

View reviewed changes

Tpt force-pushed the stream branch from 2252aef to 9287a31 Compare October 4, 2022 07:16

Migrates to json-event-parser

b30d902

Makes bindings parsing work really in streaming

Tpt force-pushed the stream branch from 9287a31 to b30d902 Compare October 4, 2022 07:18

rubensworks approved these changes Oct 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrates to json-event-parser #43

Migrates to json-event-parser #43

Tpt commented Aug 31, 2022

Tpt commented Aug 31, 2022

coveralls commented Sep 5, 2022

coveralls commented Sep 5, 2022 •

edited

Loading

rubensworks commented Sep 5, 2022

Tpt commented Sep 5, 2022

rubensworks left a comment

rubensworks Sep 5, 2022

Tpt Sep 5, 2022

surilindur commented Oct 3, 2022 •

edited

Loading

Tpt commented Oct 3, 2022

rubensworks commented Oct 4, 2022

Tpt commented Oct 4, 2022

rubensworks left a comment

rubensworks Oct 4, 2022

Tpt Oct 10, 2022 •

edited

Loading

rubensworks Oct 11, 2022

Tpt Oct 11, 2022 •

edited

Loading

LuisAverhoff commented Oct 7, 2022

Tpt commented Oct 10, 2022

rubensworks commented Oct 11, 2022

Tpt commented Oct 11, 2022 •

edited

Loading

rubensworks commented Oct 11, 2022

Tpt commented Oct 12, 2022

rubensworks commented Oct 12, 2022

Migrates to json-event-parser #43

Are you sure you want to change the base?

Migrates to json-event-parser #43

Conversation

Tpt commented Aug 31, 2022

Tpt commented Aug 31, 2022

coveralls commented Sep 5, 2022

Pull Request Test Coverage Report for Build 2993246318

💛 - Coveralls

coveralls commented Sep 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3180161106

💛 - Coveralls

rubensworks commented Sep 5, 2022

Tpt commented Sep 5, 2022

rubensworks left a comment

Choose a reason for hiding this comment

rubensworks Sep 5, 2022

Choose a reason for hiding this comment

Tpt Sep 5, 2022

Choose a reason for hiding this comment

surilindur commented Oct 3, 2022 • edited Loading

Tpt commented Oct 3, 2022

rubensworks commented Oct 4, 2022

Tpt commented Oct 4, 2022

rubensworks left a comment

Choose a reason for hiding this comment

rubensworks Oct 4, 2022

Choose a reason for hiding this comment

Tpt Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

rubensworks Oct 11, 2022

Choose a reason for hiding this comment

Tpt Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

LuisAverhoff commented Oct 7, 2022

Tpt commented Oct 10, 2022

rubensworks commented Oct 11, 2022

Tpt commented Oct 11, 2022 • edited Loading

rubensworks commented Oct 11, 2022

Tpt commented Oct 12, 2022

rubensworks commented Oct 12, 2022

coveralls commented Sep 5, 2022 •

edited

Loading

surilindur commented Oct 3, 2022 •

edited

Loading

Tpt Oct 10, 2022 •

edited

Loading

Tpt Oct 11, 2022 •

edited

Loading

Tpt commented Oct 11, 2022 •

edited

Loading