-
Notifications
You must be signed in to change notification settings - Fork 521
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
Add "pretty" json configuration and change default behavior to be space-efficient #2275
Conversation
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few questions here. Happy to meet about for a sync discussion if that seems easier.
@@ -62,6 +58,13 @@ func AssertEncoderAgainstGoldenSnapshot(t *testing.T, cfg EncoderSnapshotTestCon | |||
return | |||
} | |||
|
|||
var expected []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this code moved down. It seems like this change has no effect, which makes me think I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have explicitly labeled this change (I can split this into another PR if needed). The first iteration of this PR was heavy on snapshot testing, which after adding I backed out the change and added more explicit tests. In adding these snapshots I was surprised to see that I couldn't add any snapshots! This fixes that bug, where the snapshot needed to exist in order to update it, which is not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers, necessarily, but I'd offer a suggestion (expanded in the comment):
- don't add an escape HTML option, just never escape html
- make compact/not pretty the default
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
359df2e
to
4bb03af
Compare
@wagoodman I defer to the other reviewers for their comments being addressed here, but it looks like a cli test is failing |
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
…ce-efficient (anchore#2275) * expose underlying format options Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * remove escape html options and address PR feedback Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * incorporate PR feedback Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix cli test Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
This PR adds the following configuration options:
*-json
formats:pretty
: include indentation and newlines (default tofalse
)*-xml
formats:pretty
: include indentation at the beginning of new lines (default tofalse
)This affects the following formats:
syft-json
cyclonedx-json
cyclonedx-xml
spdx-json
A top-level
format.pretty
switch was also added that drives the default of anyformat.*.pretty
option (by default it is unset, allowing the other options to default tofalse
).This changes the default JSON output to compact JSON.
Closes #561