github / github-artifact-exporter

A set of packages to make exporting artifacts from GitHub easier
MIT License
279 stars 31 forks source link

Output valid JSON #50

Closed goncalossilva closed 3 years ago

goncalossilva commented 3 years ago

At the moment, JSON output looks like this:

{ valid_json_object }
{ valid_json_object }
{ valid_json_object }
(...)

Which, on the whole, is invalid JSON.

This PR makes it valid. If the original goal was to split issues by line, the same can still be achieved, e.g.:

process.stdout.write("[\n");
for (const issue of issues) {
  process.stdout.write(`  ${JSON.stringify(issue)},\n`);
}
process.stdout.write("]");
goncalossilva commented 3 years ago

I think that's doable. 😊

The only challenge I see is that the output is still not JSON... But it is JSON Lines. A path forward could be:

Thoughts?

jasonmacgowan commented 3 years ago

@goncalossilva that seems reasonable to me!

Chocrates commented 3 years ago

@goncalossilva can you run npm run lint:fix from the root of the project and commit those changes? Got a couple prettier errors as well. I opened another PR to fix the Super Linter error, once we get that fixed and merged your other job should magically start passing.

goncalossilva commented 3 years ago

I was a bit surprised that JSONL saves as a .jsonl file, but I suppose that makes sense.

Indeed, that is the suggestion:

JSON Lines files may be saved with the file extension .jsonl


Not something we should worry about with this one, but the fact that you had to do this a bunch of times makes me think we should refactor this smile

Haha, I considered this... But as you noted, it was out of scope. It'd be great to improve this. :+1:

can you run npm run lint:fix from the root of the project and commit those changes?

Done, apologies for missing this before!

Chocrates commented 3 years ago

@jasonmacgowan if you are happy with this I say we merge it and open a defect to get the linter and formatter aligned. I don't know why this is all of a sudden complaining.