-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 new diagnostic writer using annotate-snippet library #61407
Conversation
r? @oli-obk (rust_highfive has picked a reviewer for you, use r? to override) |
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 |
5308a62
to
3090541
Compare
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 |
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.
awesome, just some nits and maybe opinions on the ansi_term
transitive dependency.
@@ -62,6 +62,8 @@ const WHITELIST_CRATES: &[CrateVersion<'_>] = &[ | |||
const WHITELIST: &[Crate<'_>] = &[ | |||
Crate("adler32"), | |||
Crate("aho-corasick"), | |||
Crate("annotate-snippets"), | |||
Crate("ansi_term"), |
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.
cc @rust-lang/compiler we're pulling in a new dependency here, and we have no control over it: ansi_term
(annotate-snippets
is also a new dep, but it is under the rust-lang
org: https://github.com/rust-lang/annotate-snippets-rs).
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.
Two points in favour of allowing it: its license (MIT) is in our list of allowed licenses for external crates; and it is already a transitive dependency of our other core projects such as cargo
.
As long as changes to the ansi_term
are vetted as part of the annotate-snippets
crate and the crate is locked to a specific patch version I’m in favour of the dependency.
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.
The usage of ansi_term
in annotate-snippets
seems pretty lightweight so far and we could probably switch to another one as well, if that's a concern.
@bors r+ let's get this show on the road, we can always refactor if anything strong would speak against |
📌 Commit 3ab812edd1aa4e84b21888ee977a2744e65dc617 has been approved by |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
This adds a new diagnostic writer `AnnotateRsEmitterWriter` that uses the [`annotate-snippet`][as] library to print out the human readable diagnostics. The goal is to eventually switch over to using the library instead of maintaining our own diagnostics output. This commit does *not* add all the required features to the new diagnostics writer. It is only meant as a starting point so that other people can contribute as well. [as]: https://github.com/rust-lang/annotate-snippets-rs
3ab812e
to
bfe5d97
Compare
Rebased. |
@bors r=oli-obk |
📌 Commit bfe5d97 has been approved by |
Add new diagnostic writer using annotate-snippet library This adds a new diagnostic writer `AnnotateRsEmitterWriter` that uses the [`annotate-snippet`][as] library to print out the human readable diagnostics. The goal of #59346 is to eventually switch over to using the library instead of maintaining our own diagnostics output. This PR does **not** add all the required features to the new diagnostics writer. It is only meant as a starting point so that other people can start contributing as well. There are some FIXMEs in `librustc_errors/annotate_rs_emitter.rs` that point at yet to be implemented features of the new diagnostic emitter, however those are most likely not exhaustive. [as]: https://github.com/rust-lang/annotate-snippets-rs
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@5d8f59f. Direct link to PR: <rust-lang/rust#61407> 💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
This adds a new diagnostic writer
AnnotateRsEmitterWriter
that usesthe
annotate-snippet
library to print out the human readablediagnostics.
The goal of #59346 is to eventually switch over to using the library instead of
maintaining our own diagnostics output.
This PR does not add all the required features to the new
diagnostics writer. It is only meant as a starting point so that other
people can start contributing as well.
There are some FIXMEs in
librustc_errors/annotate_rs_emitter.rs
thatpoint at yet to be implemented features of the new diagnostic emitter, however
those are most likely not exhaustive.