Skip to content

feat(help): Add styling to help output #12578

Merged
bors merged 2 commits intorust-lang:masterfrom
epage:help
Sep 11, 2023
Merged

feat(help): Add styling to help output #12578
bors merged 2 commits intorust-lang:masterfrom
epage:help

Conversation

@epage
Copy link
Contributor

@epage epage commented Aug 28, 2023

What does this PR try to resolve?

Try to make --help output easier to parse by using terminal styling

Screenshots:

Screenshot from 2023-09-06 09-57-11

Screenshot from 2023-09-06 09-57-21

Screenshot from 2023-09-06 09-57-36

(nargo is my shell script wrapping cargo run --manifest-path cargo/Cargo.toml)

How should we test and review this PR?

At this time, the only styling snapshotting library I know of is a pain to use, so testing this requires manually running the commands which I did. Screenshots are included for easier evaluation of the general idea.

Snapshotting of the plain text output ensures we don't have accidental formatting regressions from this change since the formatting isn't as obvious from looking at the code.

Additional information

Traditionally, cargo has disabled clap's styled output. My assumed
reason is that cargo mixes custom help output with auto-generated and
you couldn't previously make it all styled.
Clap 4.2 allowed users to pass in strings styled using ANSI escape
codes, allowing us to pass in styled text that matches clap, unblocking this. In clap
4.4.1, clap gained the ability for the user to override the style.

In this PR, I decided to use the new 4.4.1 feature to style clap's
output to match the rest of cargo's output. Alternatively, we could use
a more subdue style that clap uses by default.

I used the color-print crate to allow something almost html-like for styling &static str. Alternatively, we could directly embed the ANSI escape codes harder to get write, harder to inspect), or we could do the styling at runtime and enable the string feature in clap.

I decided to not style Arg::help messages because

  • It might be distracting to have the descriptions lit up like a
    christmas tree
  • It is a lot more work

The one exception I made was for --list since it is for a
psuedo-command (...) and I wanted to intentionally draw attention to
it.

#12593 made styling of cargo -h cleaner imo.
#12592 and #12594 were improvements I noticed while doing this.

@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2023

r? @weihanglo

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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-run S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@epage
Copy link
Contributor Author

epage commented Aug 28, 2023

Before applying this to the rest of cargo's commands, I figured I'd post this draft for feedback from the cargo team on (1) if we want this and (2) if we should make any tweaks

@epage epage force-pushed the help branch 2 times, most recently from 3d0d622 to 0fb78ce Compare August 29, 2023 13:42
epage added a commit to epage/cargo that referenced this pull request Aug 29, 2023
In working on rust-lang#12578, I'm focusing on each help string to decide how it
should be handled and I noticed this.  It feels weird to explain
something in terms of another command's CLI, so I took `rustc --help`s
message and added `rustc` to clarify it.

In reflecting on this, I'm not 100% convinced and am open to other
opinions.
epage added a commit to epage/cargo that referenced this pull request Aug 29, 2023
In working on rust-lang#12578, I'm focusing on each help string to decide how it
should be handled and I noticed this.  It feels weird to explain
something in terms of another command's CLI, so I took `rustc --help`s
message and added `rustc` to clarify it.

Looking back, the flag was added in rust-lang#2551 with the message we have
today.  Nothing seems to really be said about it.

In reflecting on this, I'm not 100% convinced and am open to other
opinions.
epage added a commit to epage/cargo that referenced this pull request Aug 29, 2023
In working on rust-lang#12578, I felt it would be weird to style the entire
statement about commands but it also felt weird to not style it.  So
this change explores and alternatively way of communicating the
information.
epage added a commit to epage/cargo that referenced this pull request Aug 29, 2023
In working on rust-lang#12578, I felt it would be weird to style the entire
statement about commands but it also felt weird to not style it.  So
this change explores an alternatively way of communicating the
information.
bors added a commit that referenced this pull request Aug 29, 2023
fix(help): Explain --explain

In working on #12578, I'm focusing on each help string to decide how it should be handled and I noticed this.  It feels weird to explain something in terms of another command's CLI, so I took `rustc --help`s message and added `rustc` to clarify it.

Looking back, the flag was added in #2551 with the message we have today.  Nothing seems to really be said about it.

In reflecting on this, I'm not 100% convinced and am open to other opinions.
epage added a commit to epage/cargo that referenced this pull request Aug 29, 2023
Auditing all of the `--help` in prep for rust-lang#12578 and noticed that we list
the VCS information twice, once on our end and once by clap.
bors added a commit that referenced this pull request Aug 29, 2023
fix(help): Remove redundant information from new/init

Auditing all of the `--help` in prep for #12578 and noticed that we list the VCS information twice, once on our end and once by clap.
epage added a commit to epage/cargo that referenced this pull request Aug 30, 2023
In working on rust-lang#12578, I'm focusing on each help string to decide how it
should be handled and I noticed this.  It feels weird to explain
something in terms of another command's CLI, so I took `rustc --help`s
message and added `rustc` to clarify it.

Looking back, the flag was added in rust-lang#2551 with the message we have
today.  Nothing seems to really be said about it.

In reflecting on this, I'm not 100% convinced and am open to other
opinions.
bors added a commit that referenced this pull request Aug 30, 2023
fix(help): Explain --explain

In working on #12578, I'm focusing on each help string to decide how it should be handled and I noticed this.  It feels weird to explain something in terms of another command's CLI, so I took `rustc --help`s message and added `rustc` to clarify it.

Looking back, the flag was added in #2551 with the message we have today.  Nothing seems to really be said about it.

In reflecting on this, I'm not 100% convinced and am open to other opinions.
epage added a commit to epage/cargo that referenced this pull request Aug 30, 2023
In working on rust-lang#12578, I felt it would be weird to style the entire
statement about commands but it also felt weird to not style it.  So
this change explores an alternatively way of communicating the
information.
epage added a commit to epage/cargo that referenced this pull request Aug 30, 2023
In working on rust-lang#12578, I felt it would be weird to style the entire
statement about commands but it also felt weird to not style it.  So
this change explores an alternatively way of communicating the
information.
bors added a commit that referenced this pull request Aug 30, 2023
fix(help): Provide better commands heading for styling

In working on #12578, I felt it would be weird to style the entire statement about commands but it also felt weird to not style it.  So this change explores an alternatively way of communicating the information.
@bors
Copy link
Contributor

bors commented Aug 30, 2023

☔ The latest upstream changes (presumably #12593) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot
Copy link

rfcbot commented Sep 8, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@weihanglo
Copy link
Member

Let's merge it and see how people feel about it 🚀

@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2023

📌 Commit dbbc5dd has been approved by weihanglo

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2023
@bors
Copy link
Contributor

bors commented Sep 11, 2023

⌛ Testing commit dbbc5dd with merge c2d26f3...

@bors
Copy link
Contributor

bors commented Sep 11, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c2d26f3 to master...

@your-diary
Copy link

How can I disable this or change the used colors?

--color never didn't work: cargo --color never build --help

@epage
Copy link
Contributor Author

epage commented Nov 18, 2023

Could you go into why you want to disable it? It matches existing cargo colors so we thought it'd be innocuous. It'd be good to know what problems people are having with it.

As for disabling, the cli doesn't work because help gets processed before we can process --color. I can't remember if I hacked in term.color / CARGO_TERM_COLOR support but that'd be the next thing id recommend. Otherwise, as an implementation detail, we do support NO_COLOR=1 (I think there is an underscore in there).

@your-diary
Copy link

@epage

Could you go into why you want to disable it?

Because personally I don't like the colors used.
Additionally I believe there are not small number of people who want to opt-out the colorful output entirely, taking into account how many people in the world use Rust.
How colors look to one's eyes highly depends on the person.
As personal preferences of course significantly vary from person to person, I expect the command lets users customize its appearance as much as possible.
I believe this is one of the reasons why many CUI commands have --color={auto|never|always} option.
Some commands like ls even lets users customize which colorset is used (i.e. LS_COLORS).

It matches existing cargo colors

I don't think so, partly. The first impression for the current output I had was "too bright".

Actually, as you say, the color cyan has been used in Cargo. But, as far as I know, the main color is green and cyan is just temporarily displayed for highlighting purposes:

Screenshot 2023-11-18 at 22 30 37

However, in the output of --help, cyan is used as one of the main colors:

Screenshot 2023-11-18 at 22 31 02

This looks too bright for me.

As for disabling, the cli doesn't work because help gets processed before we can process --color.

I see. I suspect so. I think this is not a good UI/UX; it seems at least the doc should be fixed. The current doc is this (cargo --help and man cargo respectively):

      --color <WHEN>            Coloring: auto, always, never
       --color when
           Control when colored output is used. Valid values:

           •  auto (default): Automatically detect if color support is available on
               the terminal.

           •  always: Always display colors.

           •  never: Never display colors.

           May also be specified with the term.color config value
           <https://doc.rust-lang.org/cargo/reference/config.html>.

I can't remember if I hacked in term.color / CARGO_TERM_COLOR support but that'd be the next thing id recommend.

Didn't work either. I'd be great if the doc would be fixed for the same reason described right above.

Otherwise, as an implementation detail, we do support NO_COLOR=1

Worked like a charm. Thank you.

$ NO_COLOR=1 cargo build --help

@weihanglo
Copy link
Member

Thanks for calling them out. Some relevant contexts here about why it is hacky to support the option in CLI options and CARGO_TERM_COLOR:

That said, this is a thing I would like to see it fixed.

@epage
Copy link
Contributor Author

epage commented Nov 18, 2023

@epage

Could you go into why you want to disable it?

Because personally I don't like the colors used.

Note that we are using terminal-configured colors, rather than something like RGB, so you do have full control over how this gets rendered in your terminal.

@your-diary
Copy link

@epage
Yes. But the setting is global; it cannot (or maybe difficult if not impossible) be set on a per-command basis.

Strictly speaking, the problem is not the color itself. I feel some inconsistency in how the color is used:

Actually, as you say, the color cyan has been used in Cargo. But, as far as I know, the main color is green and cyan is just temporarily displayed for highlighting purposes:

However, in the output of --help, cyan is used as one of the main colors:

Anyway, the essential problem is all of the Rust users with different personal preferences are now forced to be exposed to the colorful output.
cargo build --color never works. cargo build --help --color never does not work.

@epage
Copy link
Contributor Author

epage commented Nov 19, 2023

Strictly speaking, the problem is not the color itself. I feel some inconsistency in how the color is used:

The reason I chose those colors for the help is that Green is used as more of a primary header while Cyan is a lower header / note.

cargo build --color never works. cargo build --help --color never does not work.

Usually a command-line flag is for transient behavior. If this is for user preference, a CLI flag wouldn't be appropriate.

As there are related issues for this., I'd recommend we shift the focus to the relevant ones as anything discussed here will be lost track of.

@your-diary
Copy link

@epage

Usually a command-line flag is for transient behavior. If this is for user preference, a CLI flag wouldn't be appropriate.

I don't think so. This is also a personal preference for design, I think.
I've learned 500 or more UNIX commands and many of them use CLI flags for user preferences.
And..., at least, cargo does support CLI flag for user preference:

function cargo() {
    if [[ $1 == 'build' ]]; then
        command cargo build --color never "$@"
    else
        command cargo "$@"
    fi
}

whether or not this is the way how --color is intended to be used.

As there are related issues for this., I'd recommend we shift the focus to the relevant ones as anything discussed here will be lost track of.

I do agree.
I just subscribed to #9012.
As #9012 is more essential, I will suspend my discussion and won't create a new issue.
For the time being, I'll use NO_COLOR=1.

Thank you for the series of the replies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment