-
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
It is impossible to understand from the readme file how to supress lints using clippy.toml #3164
Comments
It is not possible to disable lints in the For stable see also: #3159
|
So I am obligated either to have a feature called "cargo-clippy" for this: #![cfg_attr(feature = "cargo-clippy", feature(tool_lints))]
#[cfg_attr(feature = "cargo-clippy", warn(clippy::single_match))] or to use nightly. I doubt an ordinary user wants to change his code. Why can't we do that without touching the code at all? It had been okay when it was allowed to use Also, I still don't see this information in the README file which every user sees and where it can only see this information. My opinion is that it should be added there. Don't you think the opposite, do you, @flip1995 ? |
@vityafx Regarding the replacement for the |
@phansch The |
@vityafx |
@vityafx The #![cfg_attr(feature="cargo-clippy", feature(tool_lints))] allowing or denying lints was always in the code. Also the You're right about the |
It would be nice if we did not have to add anything to the source code. Okay, whatever, you'll not listen to me as a user. |
Sadly this is not possible #3159 (comment). If you have a better solution how this could be implemented, feel free to suggest it or open an Reminder: #2718 (comment) |
@vityafx I don't think clippy.toml ever supported flipping lint levels, you seem to be under the impression that we did and removed that ability. There is no "old way", the in-source attributes are the old way. The new way will eventually be through Cargo, and work is being done in that already. I'm not sure what we can do here to help you. |
@Manishearth Yes, I had thought so until you told me. Anyway, this is must have feature - to be able to use clippy without touching the code, even Cargo.toml, in my opinion. Also, it is interesting to me how you are going to deal with |
We'll probably use If you want to set lint levels on clippy without touching any code, pass |
I tried to use Clippy, on small projects it was pleasure to use (thank you for this tool), on large codebase - unusable without turning off some lints and I definitely will not switch code to nightly just for Clippy. |
I'm not sure I get what you're asking for. The tool lints stuff will stabilize soon. Even if we did implement toml stuff for Clippy that would still be nightly-only for at least as long. Wait a couple releases, or use |
We are asking for more detailed documentation: it's impossible to find in README how to allow/deny particular lints. For those who found this issue, here is my example of how to turn off lints. This code is for binary and it's placed as first lines of main.rs: #![allow(renamed_and_removed_lints)]
#![allow(unknown_lints)]
#![cfg_attr(feature = "cargo-clippy", allow(redundant_field_names,assign_op_pattern,cyclomatic_complexity,unreadable_literal,collapsible_if))]
#![cfg_attr(feature = "cargo-clippy", allow(toplevel_ref_arg,match_bool))] |
@e-oz There is also a way without touching the code, which, I think, everyone should prefer by default:
(for |
What's missing from https://github.com/rust-lang-nursery/rust-clippy/blob/master/README.md#allowingdenying-lints ? It's all there. We just don't support it in Clippy.toml. |
@Manishearth Where is the information in the README.md you have just given to us regarding |
My comment was responding to @e-oz's question about the readme. IIRC until tool_lints stabilizes I never made any statement about folks wanting to put lint levels in code, that's just the general convention in the Rust community. You're being really demanding/abrasive here, please try and be more understanding. Things are in flux right now, not everything is a deliberate decision against your use cases. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Manishearth Did not know that it is a sort of general convention, anyway, I don't understand what benefits I have if I need to change my code everytime I need some thing to check it. Also, yes, you have not made any statement, I just see that there is no such idea flying around that someone doesn't want to touch the code. Yes, I might sound offensive, I am asking for forgiveness now. I do understand that you are not obligated to do anything, but I would do that in another way, letting other people use |
I'm not. I'm trying to listen. However, you came in here asking for us to document a feature that didn't exist, and then got angry it didn't exist as if we'd made a deliberate decision not to support it. When to the contrary we had an explicit plan for it, but were holding off on things while stuff was in flux (and don't have the bandwidth to implement it ourselves right now). It's possible to ask for new features without coming off as super entitled about it. It's possible to have a nice, civil discussion about new features without assuming intent from the maintainers and yelling at them. I'm not faulting you for asking for new features -- you have a right to -- I'm asking you to be nice about it.
You don't have to change it every time. Leave the lint levels in the code. That's what everyone does, this way everyone on a project shares decisions on lint levels. It also lets you turn off lints for specific modules or functions. (This is why we moved to tool_lints, if you have |
I opened #3250 to mention However, does my comment address your use case well enough? You only have to change the code once and leave it in, not every time you wish to run the tool. If so, I'd rather not land that pull request since -A / -W aren't really common in the rust community and I'd rather wait for Cargo.toml to support these things better. |
@Manishearth #3250 is okay I think. I use The first disadvantage of disabling lints or doing something else in the code is that it will need you to recompile everything, and also, is one more place you'll have to support in the future. For example, if lint name changes, you'll also need to change it in the code. Am I wrong here? Another thing, imagine like one hundred of different tools like |
Clippy's an official rust tool, not third-party 🙂
Yeah, but that's changing, I'm not sure if I want to popularize -A if it's going to become unnecessary later. Eh, might as well merge it, we can always deprecate it later.
We don't really rename lints, and when we do we keep the old name as a redirect, IIRC. |
This is the same with command line arguments and a hypothetical
I think the main complaint I have is with the "right now". With this argument we'd never even have RFCs, FCPs and unstable features. We'd need to "move fast and break things" for every feature, which would definitely take a lot of effort on the user side
I don't see how editing the |
Code will compile even if clippy has warnings. When you work in a team and can use Clippy with config file, you don't have to create new git branch for every Clippy change, don't have to create PR, pass code review stage. It's especially true for binaries. |
CI services don't benefit from incremental compilation because, I believe, in most cases they don't store intermediate artifacts, because it is nearly useless. Also, changing your code gives you a small chance of stopping it from compiling. I have recently occured in such a problem: nightly allowed something but stable not, and there were contradictions between compiler lints and clippy lints and stable compiler. So I also made an issue about adding compiler version check macro in the But I agree with an argument that it does not matter whether I change toml file or So, basically, we came to my personal opinion of using tools that are not enabled by default in
I am not sure about this, I think I have recently seen one lint I use renamed.
Does that mean that no one else can do clippy analogs or other tools which use your code? :) My idea was to imagine a situation where every tool needs changing your code when there is a chance that it could not make you changing the code. I guess, I am out of other arguments, I got sick and so can't explain my thoughts properly, feel free to point me where I am wrong please, I will not feel offended or anything, because we always try to help each other here and I strongly believe in that. |
It is impossible to understand from the readme file how to disable some lints using clippy.toml, it seems to be explained only for nightly compiler and touching the code. I suggest to add an example of disabling some lints using clippy.toml for
rust stable
(let's takesingle_match
for example) into the readme.The text was updated successfully, but these errors were encountered: