Closed MitchellBerend closed 1 year ago
Just curious if there's a clippy
rule or similar for leftover debug printing so we could have a warning when we left some of them before committing 🤔
I don't think there is a nice way to enable a lint project wide yet. There is the option to add a clippy.toml
file but this does not allow the configuration of allowing certain specific lints.
There is an open issue on this in the main rust project. https://github.com/rust-lang/cargo/issues/5034
So, I went on a rabbit hole about the status of this issue and I was confused a lot 🥴
Here's what I've found so far (correct me if I'm wrong):
clippy
doesn't have a lint for println!("{:?}", ...)
and println!("{:#?}", ...)
. It's only for println!
entirely (and eprintln!
separately). I haven't found an issue for that in the clippy issue tracker. I imagine, lots of people using println
in their tools but don't want to use println with debugging so to me it makes sense to not forbid all prints.clippy
has a warning for the dbg!
macro. So as long as you use dbg!
instead of println!
, you should get a warningdbg!
by default? I'm confused by the clippy
website and I don't know how to make sense of it. How do I even see which checks are enabled by default and which are not?dbg!
is not a replacement for println!
because dbg!
is equivalent to println!("{:#?}", ...)
but people still could use println!("{:?}", ...)
clippy
has much more dev power than a single developer 😅)I'm not sure what can be done on the linting side here 😞
But at least, let's remove leftover printing. This can be done 😅
I'd like to help with this
@chshersh I found a way to run clippy any clippy lints. cargo clippy --help
shows 4 flags that correspond with the warning levels. I propose this gets added to ci and the pre-commit file. If i run cargo clippy -- -D clippy::print_stdout
the output is:
Checking tool-sync v0.2.0 (/home/mitchell/rust/tool-sync)
error: use of `println!`
--> src/lib.rs:26:5
|
26 | println!("asdasd");
| ^^^^^^^^^^^^^^^^^^
|
= note: requested on the command line with `-D clippy::print-stdout`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#print_stdout
error: use of `println!`
--> src/config/schema.rs:55:9
|
55 | println!("asdfasdf");
| ^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#print_stdout
error: use of `println!`
--> src/infra/client.rs:28:9
|
28 | println!("{:?}", &self.proxy);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#print_stdout
error: use of `println!`
--> src/infra/client.rs:38:9
|
38 | println!("{:?}", &self.proxy);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#print_stdout
error: could not compile `tool-sync` due to 4 previous errors
Edit: I added a few extra garbage println
calls in the code and I annotated the correct ones with #[allow(clippy::print_stdout)]
@MitchellBerend The issue is about removing extra calls and it's now resolved by @jim4067 in #152.
Could you open a separate issue about clippy warnings so we don't lose this useful info until we improve our DX? I didn't have the chance to look into it yet 😞
Also, IIUC, it should be enough to put these clippy options only in src/lib.rs
to apply them to the entire tool-sync
which is not that bad actually. So maybe we can do this instead of patching our CI config 🤔
Also, IIUC, it should be enough to put these clippy options only in src/lib.rs
I thought I tried this and it didn't work. I'll look into this though and if I can't get it working ill open a new issue.
https://github.com/chshersh/tool-sync/blob/395b21c5d39ac4e39b492657954eb283319b2291/src/infra/client.rs#L27-L57
These 2 functions have left over
println
calls from development. These look really weird when syncing a bunch of tools, instead of reusing the first shell linetool
will print outNone
or the proxy passed in and continue to do that until all info is fetched.