Add colored help to be consistent with Cargo #11495

blyxyas · 2023-09-13T17:51:43Z

On rust-lang/cargo#12578, a new colored help message format was introduced. This PR introduces the same styling from that cargo help message to our cargo clippy --help message.

More information is provided in the original PR, fixes #11482. The perfect reviewing process would be that the reviewer installs this branch and checks for themselves, but here are some screenshots, there are some more screenshots in the original issue.

(Note that the actual text may change in the actual commit, that screenshot is just to test the colors).
Also note that the color-print version should always be synced with Cargo's color-print version to avoid increasing build times in the rust-lang/rust repo.

changelog:Add colors to the cargo clippy --help output 🎉.

rustbot · 2023-09-13T17:51:47Z

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

src/driver.rs

src/main.rs

flip1995

Blocker: This doesn't work when piping the output to a file:

LD_LIBRARY_PATH=$(rustc --print=sysroot)/lib ./target/debug/cargo-clippy --help > test

results in:

Checks a package to catch common mistakes and improve your Rust code.

�[32m�[1mUsage�[39m�[22m:
    �[36m�[1mcargo clippy�[39m�[22m �[36m[OPTIONS] [--] [<ARGS>...]�[39m

�[32m�[1mCommon options:�[39m�[22m
    �[36m�[1m--no-deps�[39m�[22m                Run Clippy only on the given crate, without linting the dependencies
    �[36m�[1m--fix�[39m�[22m                    Automatically apply lint suggestions. This flag implies �[36m--no-deps�[39m and �[36m--all-targets�[39m
    �[36m�[1m-h, --help�[39m�[22m               Print this message
    �[36m�[1m-V, --version�[39m�[22m            Print version info and exit
    �[36m�[1m--explain [LINT]�[39m�[22m           Print the documentation for a given lint

See all options with �[36m�[1mcargo check --help�[39m�[22m.

�[32m�[1mAllowing / Denying lints�[39m�[22m

To allow or deny a lint from the command line you can use �[36m�[1mcargo clippy --�[39m�[22m with:

    �[36m�[1m-W�[39m�[22m / �[36m�[1m--warn�[39m�[22m �[36m[LINT]�[39m       Set lint warnings
    �[36m�[1m-A�[39m�[22m / �[36m�[1m--allow�[39m�[22m �[36m[LINT]�[39m      Set lint allowed
    �[36m�[1m-D�[39m�[22m / �[36m�[1m--deny�[39m�[22m �[36m[LINT]�[39m       Set lint denied
    �[36m�[1m-F�[39m�[22m / �[36m�[1m--forbid�[39m�[22m �[36m[LINT]�[39m     Set lint forbidden

You can use tool lints to allow or deny lints from your code, e.g.:

    �[33m�[1m#[allow(clippy::needless_lifetimes)]�[39m�[22m

blyxyas · 2023-09-14T08:09:49Z

OK, this new approach fixes Philipp's comment but introduces kind of a lot of bloat. On the original PR, this bloat isn't there because clap handles a lot of minute things (and removing formatting when piping to a file is one of them, I think). But as we don't use clap, we have to manually check where output is going.

flip1995 · 2023-09-14T08:25:55Z

But as we don't use clap, we have to manually check where output is going.

Should we use clap? Should we nominate this PR for discussion?

blyxyas · 2023-09-14T08:47:43Z

Adding clap as a dependency would add 55Kb, but it adds some QOL improvements and overall more readability. I think nominating this is the best option, as the pros and cons are pretty similar and subjective, so we'll have to know what the team thinks.

Also note that, if we use clap, the kinda clunky syntax of cargo clippy -- -W [LINT] would change to a slightly better cargo clippy -W [LINT]

flip1995 · 2023-09-14T09:25:25Z

the kinda clunky syntax of cargo clippy -- -W [LINT] would change to a slightly better cargo clippy -W [LINT]

Sadly the clunky syntax is probably there to stay for backwards compat...

blyxyas · 2023-09-14T09:49:20Z

There's probably a way to maintain this backwards compatibility while also providing a better solution.

epage · 2023-09-15T23:08:01Z

As an alternative to pulling in all of clap, you can use the anstream library that clap uses that wraps stdout/stderr with the ability to strip ansi escape codes when needed.

blyxyas · 2023-09-16T08:09:25Z

The current commit also checks if we're on a terminal and adjusts the output to that information. Pulling a dependency just to avoid an if check would not be the best decision (it would also hurt runtime performance)

epage · 2023-09-16T13:04:27Z

Help output performance isn't the biggest deal and the dependency helps with (1) only having one instance of the help to avoid subtle issues with them being out of sync and (2) it covers more situations (NOCOLOR, CLI_COLOR, legacy windows, etc).

flip1995

You found a bug in ignored_unit_patterns: Needs an external macro check.

Cargo.toml

src/driver.rs

blyxyas · 2023-09-20T11:36:37Z

Needs an external macro check.

The lint uses check_pat, and sadly we cannot use is_from_proc_macro with hir::Pats yet. I'll add it to the backlog for when the is_from_proc_macro update is merged.

src/driver.rs

flip1995 · 2023-09-20T13:40:46Z

src/main.rs

+    <cyan,bold>--fix</>                    Automatically apply lint suggestions. This flag implies <cyan>--no-deps</> and <cyan>--all-targets</>
+    <cyan,bold>-h</>, <cyan,bold>--help</>               Print this message
+    <cyan,bold>-V</>, <cyan,bold>--version</>            Print version info and exit
+    <cyan,bold>--explain [LINT]</>           Print the documentation for a given lint


Suggested change

<cyan,bold>--explain [LINT]</> Print the documentation for a given lint

<cyan,bold>--explain [LINT]</> Print the documentation for a given lint

flip1995

Still some work to be done on the clippy-driver help message.

src/driver.rs

flip1995 · 2023-09-21T08:42:14Z

r? @flip1995

src/driver.rs

flip1995 · 2023-09-22T08:11:06Z

Please fix the alignment of the --explain and the --rustc flags and this should be good to go.

Optionally address this comment: #11495 (comment)

flip1995 · 2023-09-22T08:12:32Z

src/driver.rs

+#[must_use]
+fn help_message() -> &'static str {
+    color_print::cstr!(
+"Checks a package to catch common mistakes and improve your Rust code. Run <cyan>clippy-driver</> with the same arguments you use for <cyan>rustc</>


Suggested change

"Checks a package to catch common mistakes and improve your Rust code. Run <cyan>clippy-driver</> with the same arguments you use for <cyan>rustc</>

"Checks a file to catch common mistakes and improve your Rust code.

Run <cyan>clippy-driver</> with the same arguments you use for <cyan>rustc</>

blyxyas · 2023-09-22T09:10:09Z

Optionally address this comment: #11495 (comment)

#[allow] attributes cannot be applied println! since it's a macro invocation.

error: unused attribute `allow`
   --> src/driver.rs:217:13
    |
217 |             #[allow(clippy::ignored_unit_patterns)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: the built-in attribute `allow` will be ignored, since it's applied to the macro invocation `println`
   --> src/driver.rs:218:13
    |
218 |             println!("{version_info}");
    |

Truly a sad cowboy moment.

src/driver.rs

flip1995

Thanks! r=me after squashing the commits 🚀

blyxyas · 2023-09-25T11:34:25Z

@bors r=flip1995

bors · 2023-09-25T11:34:28Z

📌 Commit 6ad218c has been approved by flip1995

It is now in the queue for this repository.

bors · 2023-09-25T11:35:35Z

⌛ Testing commit 6ad218c with merge 78ddc8d...

bors · 2023-09-25T11:47:44Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 78ddc8d to master...

rustbot assigned giraffate Sep 13, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 13, 2023

smoelius reviewed Sep 13, 2023

View reviewed changes

src/driver.rs Outdated Show resolved Hide resolved

giraffate approved these changes Sep 14, 2023

View reviewed changes

src/driver.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

flip1995 requested changes Sep 14, 2023

View reviewed changes

blyxyas added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Sep 14, 2023

flip1995 reviewed Sep 20, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/driver.rs Outdated Show resolved Hide resolved

flip1995 requested changes Sep 20, 2023

View reviewed changes

blyxyas force-pushed the help_message_reformat branch from 2f1f8df to 6ed217f Compare September 20, 2023 14:07

flip1995 requested changes Sep 21, 2023

View reviewed changes

src/driver.rs Outdated Show resolved Hide resolved

src/driver.rs Outdated Show resolved Hide resolved

rustbot assigned flip1995 and unassigned giraffate Sep 21, 2023

flip1995 reviewed Sep 21, 2023

View reviewed changes

src/driver.rs Show resolved Hide resolved

flip1995 reviewed Sep 22, 2023

View reviewed changes

src/driver.rs Show resolved Hide resolved

flip1995 approved these changes Sep 25, 2023

View reviewed changes

Add colored help

6ad218c

blyxyas force-pushed the help_message_reformat branch from efe28ee to 6ad218c Compare September 25, 2023 11:33

bors merged commit 78ddc8d into rust-lang:master Sep 25, 2023
4 checks passed

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Oct 3, 2023

blyxyas deleted the help_message_reformat branch October 5, 2023 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add colored help to be consistent with Cargo #11495

Add colored help to be consistent with Cargo #11495

blyxyas commented Sep 13, 2023

rustbot commented Sep 13, 2023

flip1995 left a comment

blyxyas commented Sep 14, 2023

flip1995 commented Sep 14, 2023 •

edited

Loading

blyxyas commented Sep 14, 2023

flip1995 commented Sep 14, 2023

blyxyas commented Sep 14, 2023

epage commented Sep 15, 2023

blyxyas commented Sep 16, 2023

epage commented Sep 16, 2023

flip1995 left a comment

blyxyas commented Sep 20, 2023 •

edited

Loading

flip1995 Sep 20, 2023

flip1995 left a comment

flip1995 commented Sep 21, 2023

flip1995 commented Sep 22, 2023

flip1995 Sep 22, 2023

blyxyas commented Sep 22, 2023

flip1995 left a comment

blyxyas commented Sep 25, 2023

bors commented Sep 25, 2023

bors commented Sep 25, 2023

bors commented Sep 25, 2023

	<cyan,bold>--explain [LINT]</> Print the documentation for a given lint
	<cyan,bold>--explain [LINT]</> Print the documentation for a given lint

	"Checks a package to catch common mistakes and improve your Rust code. Run <cyan>clippy-driver</> with the same arguments you use for <cyan>rustc</>
	"Checks a file to catch common mistakes and improve your Rust code.
	Run <cyan>clippy-driver</> with the same arguments you use for <cyan>rustc</>

Add colored help to be consistent with Cargo #11495

Add colored help to be consistent with Cargo #11495

Conversation

blyxyas commented Sep 13, 2023

rustbot commented Sep 13, 2023

flip1995 left a comment

Choose a reason for hiding this comment

blyxyas commented Sep 14, 2023

flip1995 commented Sep 14, 2023 • edited Loading

blyxyas commented Sep 14, 2023

flip1995 commented Sep 14, 2023

blyxyas commented Sep 14, 2023

epage commented Sep 15, 2023

blyxyas commented Sep 16, 2023

epage commented Sep 16, 2023

flip1995 left a comment

Choose a reason for hiding this comment

blyxyas commented Sep 20, 2023 • edited Loading

flip1995 Sep 20, 2023

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Sep 21, 2023

flip1995 commented Sep 22, 2023

flip1995 Sep 22, 2023

Choose a reason for hiding this comment

blyxyas commented Sep 22, 2023

flip1995 left a comment

Choose a reason for hiding this comment

blyxyas commented Sep 25, 2023

bors commented Sep 25, 2023

bors commented Sep 25, 2023

bors commented Sep 25, 2023

flip1995 commented Sep 14, 2023 •

edited

Loading

blyxyas commented Sep 20, 2023 •

edited

Loading