Emit ansi color codes in the rendered field of json diagnostics
#59128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I've been looking at doing something similar to this. I'm considering using JSON output only for Cargo for a few reasons. Do you happen to know if the "rendered" field is fully consistent with the normal output? |
This comment has been minimized.
This comment has been minimized.
Yes it's identical. It's actually generated by invoking the regular diagnostic renderer and just recording the output. |
|
@oli-obk Can we get |
|
Oh yea, totally forgot about this. I've been meaning to fix that in forever. I'll do that in this PR as to not regress the status quo even further. |
|
My email is telling me I was r?-ed, but I can't see myself in this thread... am I missing something? |
|
the |
mark-i-m
approved these changes
Mar 12, 2019
Note that this would not have changed the rendered output (because that would severely screw with r? @eddyb |
This comment has been minimized.
This comment has been minimized.
eddyb
reviewed
Mar 13, 2019
| "failed to decode compiler output as json: \ | ||
| `{}`\nline: {}\noutput: {}", | ||
| error, line, output | ||
| ))); | ||
| ); | ||
| panic!() |
There was a problem hiding this comment.
Does this panic! match what proc_res.fatal would do?
Nevermind, it avoids fatal trying to print the rendered field from JSON (and failing).
eddyb
reviewed
Mar 13, 2019
| self.status, self.cmdline, self.stdout, self.stderr | ||
| self.status, self.cmdline, | ||
| json::extract_rendered(&self.stdout), | ||
| json::extract_rendered(&self.stderr), |
There was a problem hiding this comment.
I assume this is the main compiletest change? 🎉
|
@bors r=mark-i-m,eddyb |
|
📌 Commit 8f1d62befd2db8bfbe0eef0cd163945fafde2bf4 has been approved by |
bors
added
the
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
|
@bors r- |
bors
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
|
@bors r=mark-i-m,eddyb |
|
📌 Commit b5fc2c41b39c666fce1c275690a79bdc9cdc33d0 has been approved by |
bors
added
S-waiting-on-bors
estebank
reviewed
Mar 14, 2019
| @@ -1,5 +1,5 @@ | |||
| // ignore-cloudabi | |||
| // compile-flags: --error-format pretty-json -Zunstable-options | |||
| // compile-flags: --error-format pretty-json -Zunstable-options --colorful-json=true | |||
There was a problem hiding this comment.
I don't see a test for the current output here :-/
Also, although I see the immediate benefit of this, I would like for us to tackle a more "general" solution at some point where we can also produce HTML, for example.
There was a problem hiding this comment.
We do have other --error-format pretty-json ui tests.
So you're imagining something like --json-rendered=(plain|termcolor|html)?
There was a problem hiding this comment.
In my mind something like --json-rendered=extended where the textual output would be the same and we'd have extra json fields annotating those with extra information:
{"text": "path `std::string::String`", "annotations": [{"start": 6, "end": 25, "highlight_type": "highlighted"}]}
That way any tool can consume these and turn them into whatever their needs may be.
That being said, having specific support for both terminal and html output make sense to me, given they probably cover 80% of all likely use cases.
There was a problem hiding this comment.
The --json-rendered list can be extended in the future to support schemes like the one you proposed. I will change the PR to only support plain and termcolor for now.
|
🌲 The tree is currently closed for pull requests below priority 15, this pull request will be tested once the tree is reopened |
|
⌛ Testing commit 325936a with merge 1d700581f3bce5188c75e2e789d93ea6b97acfed... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
I guess color codes differ between windows and linux? |
kennytm
added
S-waiting-on-author
|
@bors r- |
|
@bors r=mark-i-m,eddyb |
|
📌 Commit 55534e41dda3847c605f56a32b6e7d5b1ea483c5 has been approved by |
bors
added
S-waiting-on-bors
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r- |
bors
added
S-waiting-on-author
|
@bors r=mark-i-m,eddyb |
|
📌 Commit 5c6a43a has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Apr 17, 2019
…ddyb Emit ansi color codes in the `rendered` field of json diagnostics cc @ljedrz Implemented for rust-lang#56595 (comment) (x.py clippy)
bors
added a commit
that referenced
this pull request
Apr 17, 2019
Rollup of 5 pull requests Successful merges: - #59128 (Emit ansi color codes in the `rendered` field of json diagnostics) - #59646 (const fn: Improve wording) - #59986 (Miri: refactor new allocation tagging) - #60003 (LLD is not supported on Darwin) - #60018 (Miri now supports entropy, but is still slow) Failed merges: r? @ghost
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @ljedrz
Implemented for #56595 (comment) (x.py clippy)