-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Check for literals as println! args #2608
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
Check for literals as println! args #2608
Conversation
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 very clean, nice. Just a nit due to recent changes in our system.
Could you also add a test that ensures that println!("foo: {}", env!("BAR"));
does not lint?
clippy_lints/src/print.rs
Outdated
/// ```rust | ||
/// println!("{}", "foo"); | ||
/// ``` | ||
declare_lint! { |
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.
We're deprecating the use of this macro. Please use declare_clippy_lint
together with the nursery group
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 is fixed in 511aa65.
Good catch on the Currently running into some weirdness when running |
Sometimes you need to rerun all tests, run the script, run tests again, run the script before the update script catches on. This also happens inside rustc... no clue what's exactly happening. |
You can probably fix the env! issue with very targetted in_macro calls. Not sure how that'll work, since println is also a macro. But technically the args should not be inside a macro expansion |
16a88a6
to
511aa65
Compare
Yeah, looked like things were out of sync. A |
Do you think this should be pulled out of print.rs or run as an In the test |
Ah, thanks for investigating. Can you just list this issue under the downsides of the lint? It's fine to have this weird corner case imo |
Sounds good. I added the test-case and added the issue to "Known Problems." Let me know if there's anything else you'd like to see in this. |
To make the tests reproducible (on all platforms) (and without explicitly setting an environment var), I removed the test with the |
Nope, this is fine as it is. Thanks |
Addresses #2480