Add support for JSON formatting / Always use stderr for error messages#5
Add support for JSON formatting / Always use stderr for error messages#5egrtechno merged 5 commits intoTechnolution:masterfrom cipriancraciun:master
stderr for error messages#5Conversation
stderr for error messagesstderr for error messages
|
Thanks for your pull request. I'm very happy that you find our tool useful and that you added a feature we also wanted. I'll take a look at it shortly! |
| impl OutputStream for JsonConsoleOutputStream { | ||
| fn print_output(&self, panic_calls: &PanicCallsCollection) { | ||
| let stream = io::stdout(); | ||
| for (i, trace) in panic_calls.calls.iter().enumerate() { |
There was a problem hiding this comment.
We are missing an outer Json Array here. Some tools consider the output invalid JSON.
There was a problem hiding this comment.
I think this should be considered as a separate option.
For example the best way to handle large JSON files, especially when the "root" object is a large array of similar objects, is to render them as a "stream" of JSON objects. (For example the jq tool works like this natively, and there is also an RFC describing this https://tools.ietf.org/html/rfc7464 )
In the end the difference is minimal, and I can provide the patch for this extra version:
--json would create that large array;
--json-sequence would create the format I've already proposed;
There was a problem hiding this comment.
Okay, I was not aware of the spec. Does the current implementation indeed implement the spec, which says: JSON-sequence = *(RS JSON-text LF)?
There was a problem hiding this comment.
Unfortunately no, it doesn't use the record separator character, mainly because I thought having this "in-between" format is "good-enough", especially since works by default with jq, and it can easily be changed with a sed script into an actual JSON-object or JSON-sequence.
Moreover fully implementing that RFC would require one to lose all the pretty-printing.
Therefore perhaps the option --json-sequence should output a format compliant with that RFC, and another one --json-stream could retain the current "good-enough" format.
There was a problem hiding this comment.
I agree with your reasoning. Let's just change to option name and be done with it.
There was a problem hiding this comment.
Then just call it --json-stream which I guess it's the best "description".
| let json = json!({ | ||
| "index" : i, | ||
| "pattern" : match trace.pattern.borrow().deref() { | ||
| PanicPattern::Unrecognized => "unrecognized", |
There was a problem hiding this comment.
Hmm, is this the best place to do this translation, or can we use the Display trait for this?
There was a problem hiding this comment.
I would say it's better to have all JSON-specific formatting explicit, especially since it allows the Display format to change without breaking the JSON rendering.
(I.e. you could view JSON as an "external" machine-readable format, that shouldn't change in backward-compatible ways.)
|
Sorry, wrong button. Did not want to approve the entiry PR. I'm new to using GitHub. |
egrtechno
left a comment
There was a problem hiding this comment.
I think we need to fix some Json output details.
The first small patch replaces
println!witheprintln!inmainso that errors are printed tostderr, and thus allowing one to better integraterustiginto other pipelines.The other four patches add support for JSON formatting of the analysis.
The format used is in fact a "JSON-stream" (i.e. one object after another separated by empty lines), which are perfect for consumption by the
jqtool.For example if one dumps the contents of the analysis of
rustigitself into a JSON file, then one can execute the followingjqquery that prints all the "top" functions that might cause a panic, but whose "pattern" is not trivial (like unwrap, indexing, or airthmetic):