Left over debug println in proxy implementation should be removed

[MitchellBerend]

tool-sync/src/infra/client.rs

Lines 27 to 57 in 395b21c

	fn asset_url(&self, asset_id: u32) -> String {
	println!("{:?}", &self.proxy);
	format!(
	"https://api.github.com/repos/{owner}/{repo}/releases/assets/{asset_id}",
	owner = self.owner,
	repo = self.repo,
	asset_id = asset_id
	)
	}
	pub fn fetch_release_info(&self) -> Result<Release, Box<dyn Error>> {
	println!("{:?}", &self.proxy);
	let release_url = self.release_url();
	let req = match &self.proxy {
	Some(proxy) => {
	let agent = ureq::AgentBuilder::new().proxy(proxy.clone()).build();
	add_auth_header(
	agent
	.get(&release_url)
	.set("Accept", "application/vnd.github+json")
	.set("User-Agent", "chshersh/tool-sync-0.2.0"),
	)
	}
	None => add_auth_header(
	ureq::get(&release_url)
	.set("Accept", "application/vnd.github+json")
	.set("User-Agent", "chshersh/tool-sync-0.2.0"),
	),
	};

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 line tool will print out None or the proxy passed in and continue to do that until all info is fetched.

[chshersh]

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 🤔

[MitchellBerend]

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 thinking

These exist but they will also trigger on correct println calls
stderr
stdout

[MitchellBerend]

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.
rust-lang/cargo#5034

[chshersh]

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.
• However, clippy has a warning for the dbg! macro. So as long as you use dbg! instead of println!, you should get a warning
• Unless... It seems there's no warning on dbg! 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?
• Moreover, dbg! is not a replacement for println! because dbg! is equivalent to println!("{:#?}", ...) but people still could use println!("{:?}", ...)
• Also, it looks like indeed there's no way to disable/enable lints from a single configuration file and you need to enable/disable it in every single crate (aka file) (I was really surprised by this one as I wrote a static analyser in Haskell before and I had support for lints from a file but surely 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 😅

[jim4067]

I'd like to help with this

[MitchellBerend]

@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)]

[chshersh]

@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 🤔

[MitchellBerend]

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.