-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Convert a few more diagnostics to reports #16066
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
Conversation
unused_fields.join(", ") | ||
)) | ||
.element(Level::NOTE.message(format!( | ||
"configure {} in the `dependencies` entry", |
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.
Slightly off-topic, but I wanted to attach a manifest/config location here. It's a bit tricky, though, because the dependency table was the result of a merge between configs and manifests. How would you feel about having Dependency
(and potentially other things) keep track of their source location?
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.
Really look forward to us adding more context like this though don't feel obligated to do so. This effort is more prep work for us to start doing these kinds of improvements. The most I expected to be done with this is wording changes to better fit the new style.
|
@jneem do you have a preference for which route we go? |
Not really, I'll happily continue with whichever you prefer. |
I think @Muscraft was leaning towards this one. Mind switching it to Also, feel free to change |
Sounds good, I converted this to secondary_title and changed a couple notes to helps. |
Thanks! |
Here I'm continuing to slowly work through multi-part diagnostics and convert them to reports.
Like #16035, this converts a warning + note into a single
Group
with a warning title containing note elements. Based on this comment I'm not sure if this is actually what you want, mostly because these notes are typically shorter than the ones in #16035.For comparison, I'll open another PR that does multiple groups. Edit: #16065
This is part of #15944