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

machine readable output #212

Merged
merged 7 commits into from
Jun 11, 2022
Merged

machine readable output #212

merged 7 commits into from
Jun 11, 2022

Conversation

chris48s
Copy link
Owner

@chris48s chris48s commented Jun 5, 2022

refs #148

TODO

  • write some tests
  • documentation
  • possibly SARIF, but maybe do it in another PR

chris48s added 4 commits June 5, 2022 15:39
This commit does several things:

1. Send all the log/debug/error/success messages to stderr
   This lays the groundwork for making anything we send to
   stdout machine-parseable (in one way or another)
2. Change mocking strategy under test
   By mocking logger.writeOut and logger.writeErr under test
   to supress console output under test
   (rather than mocking `console.log()` and friends directly),
   this makes it easier to quickly `console.log()` something
   while debugging a failing test but still supress most output
3. Explicitly pass AJV `strict: false` or `strict: "log"`
   based on verbosity level instead of mocking console methods.
4. Only output AJV strict log messages when verbosity >= 2
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2022

Codecov Report

Merging #212 (bd7b0c5) into main (f4dcb0d) will decrease coverage by 0.88%.
The diff coverage is 90.30%.

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
- Coverage   95.12%   94.24%   -0.89%     
==========================================
  Files          10       11       +1     
  Lines         657      730      +73     
  Branches      149      160      +11     
==========================================
+ Hits          625      688      +63     
- Misses         32       40       +8     
- Partials        0        2       +2     
Impacted Files Coverage Δ
src/glob.js 76.19% <50.00%> (ø)
src/cli.js 91.27% <80.95%> (-4.85%) ⬇️
src/output-formatters.js 84.21% <84.21%> (ø)
src/logger.js 93.75% <93.75%> (ø)
src/ajv.js 100.00% <100.00%> (ø)
src/cache.js 97.43% <100.00%> (ø)
src/catalogs.js 95.79% <100.00%> (+0.22%) ⬆️
src/config.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4dcb0d...bd7b0c5. Read the comment docs.

src/config.js Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
if (config.format === "json") {
logger.log(JSON.stringify({ results }, null, 2));
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

if (config.format === "sarif") {
...

src/config.js Outdated Show resolved Hide resolved
@chris48s chris48s changed the title machine readable output WIP machine readable output Jun 5, 2022
@chris48s
Copy link
Owner Author

chris48s commented Jun 9, 2022

Assuming I punt SARIF format to another issue, I think the one remaining thing to deal with here is array vs object. i.e:

  • JSON.stringify({ results }, null, 2) or
  • JSON.stringify({ results: Object.values(results) }, null, 2)

I think for parsing in programming languages it doesn't make a huge amount of difference. The other likely way to parse this will be piping to jq. I think I will do a quick spike on parsing the output with jq to see if the ergonomics of that obviously point to one approach or the other.

@chris48s
Copy link
Owner Author

Doesn't make a huge difference for parsing it with jq. If its an array the query to get all schemaLocations will be like

  • jq .results[].schemaLocation

If its an object, the equivalent is:

  • jq '.results | to_entries[] | .value.schemaLocation'

To get schemaLocation for a specific result, if its an array:

  • jq '.results[] | select(.fileLocation == "./testfiles/files/invalid.json").schemaLocation'

For an object it would be:

  • jq '.results["./testfiles/files/invalid.json"].schemaLocation'

So.. little from column A. Little from column B/

Another consideration is that something like ./testfiles/files/invalid.json is quite a cumbersome property name.

I probably don't need to paginate this (its not an API), but making it an array works better for that.

There's not much to choose here but I think I'm going to go with the object. At least then the internal data structure and output file are the same.

@chris48s chris48s changed the title WIP machine readable output machine readable output Jun 11, 2022
@chris48s chris48s marked this pull request as ready for review June 11, 2022 18:02
@chris48s chris48s merged commit 603c76d into main Jun 11, 2022
@chris48s
Copy link
Owner Author

Well it seemed like a good idea at the time 🙄

v8r/CHANGELOG.md

Lines 5 to 6 in e3f23b4

* **Breaking:** Change to the JSON output format. The `results` key is now an array instead of an object.
In v8r <4, `results` was an object mapping filename to result object. For example:

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.

2 participants