-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve "unknown field" error messages #8823
Conversation
The output looks very cool ❤️ the dependency should be fine if it's also used by This is also worthy of a changelog 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.
The output already looks good to me 👍
src/main.rs
Outdated
@@ -123,8 +123,12 @@ impl ClippyCmd { | |||
.map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) | |||
.collect(); | |||
|
|||
// Currently, `CLIPPY_TERMINAL_WIDTH` is used only to format "unknown field" error messages. | |||
let terminal_width = termize::dimensions_stdin().map_or(0, |(w, _)| w); |
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.
Is there any advantage in getting the width here and storing it in an environment value? This will not be used 99% of the time. Could we move this to the warning code? Having an environment value is usually intended to enable users to configure or read something, but neither applies here.
If there is a reason for this, I'm fine with it 🙃
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 think Cargo is piping rustc's IO, though I haven't been able to figure out precisely where this occurs (in the jobserver crate is my best guess). When I tried to do as you suggested, I got None
from termize::dimensions()
every time. I even tried at the very start of the driver's main
function, but the result was the same.
As I was typing this, I realized that dimensions_stdin
should be dimensions
in the line you cited, hence the forced push. (dimensions_stdin
was a relic from experimenting with this very issue.)
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.
Good to know that might also come in handy for a different project 🙃
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.
As I was typing this, I realized that dimensions_stdin should be dimensions in the line you cited, hence the forced push. (dimensions_stdin was a relic from experimenting with this very issue.)
Thanks for the hint. GitHub has finally improved their force push information so that it's possible to see the specific changes of a force push. 🥳
Looks good to me, thank you for the update! I've changed the changelog slightly, hope that's okay with you 🙃 @bors r+ |
📌 Commit 5647257 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
||
let mut msg = String::from(prefix); | ||
for row in 0..rows { | ||
write!(msg, "\n").unwrap(); |
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.
This looks like a typical clippy lint: use writeln
instead 😁
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.
Ironically, on lines 422, 426, and 437, I originally had s += ...
. Then, format_push_string
fired, and I changed them to write!
.
I don't know why write_with_newline
doesn't fire here. But to be honest, I'm kind of glad it doesn't. My aesthetic self kind of prefers the symmetry with lines 426 and 437. 🤷
Fixes #8806
Sample output:
You can test this by (say) adding
foobar = 42
to Clippy's rootclippy.toml
file, and runningcargo run --bin cargo-clippy
.Note that, to get the terminal width, this PR adds
termize
as a dependency tocargo-clippy
. However,termize
is also howrustc_errors
gets the terminal width. So, hopefully, this is not a dealbreaker.r? @xFrednet
changelog: Enhancements: the "unknown field" error messages for config files now wraps the field names.