Skip to content
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

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

Merged
merged 2 commits into from
Sep 11, 2023
Merged

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

merged 2 commits into from
Sep 11, 2023

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.

epage added a commit to epage/clap-cargo that referenced this pull request Sep 11, 2023
This mirrors what the API in rust-lang/cargo#12655.  I'm hoping to have
the `anstyle` consts be a public crate owned by the cargo team in the
future after which we'll refactor to re-export it.

See rust-lang/cargo#12578 for adding the style.
epage added a commit to epage/clap-cargo that referenced this pull request Sep 11, 2023
This mirrors what the API in rust-lang/cargo#12655.  I'm hoping to have
the `anstyle` consts be a public crate owned by the cargo team in the
future after which we'll refactor to re-export it.

See rust-lang/cargo#12578 for adding the style.
bors added a commit that referenced this pull request Sep 13, 2023
refactor: Consolidate clap/shell styles

### What does this PR try to resolve?

This is a follow up to #12578 to reduce duplication of terminal styling.

This still leaves styling in `color_print::cstr!`.

This is also prep for copy/pasting into `clap-cargo` for others to use.  Another step might be to extract `cargo::util::style` into its own crate.

### How should we test and review this PR?

We have no styling tests so this can only be verified by inspection or running the commands

### Additional information

I chose `anstyle` for expressing these as its an API designed specifically for stable style definitions to put in public APIs.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2023
Update cargo

11 commits in 2fc85d15a542bfb610aff7682073412cf635352f..d5336f813df39d476e61fc46daabb1446350660a
2023-09-09 01:49:46 +0000 to 2023-09-14 19:55:49 +0000
- fix: emit a warning for `credential-alias` shadowing (rust-lang/cargo#12671)
- refactor: fix lint errors in preparation of `[lints]` table integration (rust-lang/cargo#12669)
- Limit cargo add feature print (rust-lang/cargo#12662)
- Clippy (rust-lang/cargo#12667)
- Prerelease candidates error message (rust-lang/cargo#12659)
- Fix typos: `informations` -> `information` (rust-lang/cargo#12666)
- chore: update globset to 0.4.13 (rust-lang/cargo#12665)
- refactor: Consolidate clap/shell styles (rust-lang/cargo#12655)
- libgit2 fixed upstream (rust-lang/cargo#12657)
- Index summary enum (rust-lang/cargo#12643)
- feat(help): Add styling to help output (rust-lang/cargo#12578)

r? ghost
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Sep 18, 2023
bors added a commit to rust-lang/rust-clippy that referenced this pull request Sep 25, 2023
Add colored help to be consistent with Cargo

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.

![image](https://github.com/rust-lang/rust-clippy/assets/73757586/0c4d1b6d-5aa2-4146-a401-9ae88f6dedf5)

(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 🎉.
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
@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.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 8, 2024
Pkgsrc changes:

 * Remove NetBSD-8 support (embedded LLVm requires newer C++
   than what is in -8; it's conceivable that this could still
   build with an external LLVM)
 * undo powerpc 9.0 file naming tweak, since we no longer support -8.
 * Remove patch to LLVM for powerpc now included by upstream.
 * Minor adjustments, checksum changes etc.


Upstream changes:

Version 1.74.1 (2023-12-07)
===========================

- [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM]
  (rust-lang/rust#118464)
- [Clarify guarantees for std::mem::discriminant]
  (rust-lang/rust#118006)
- [Fix some subtyping-related regressions]
  (rust-lang/rust#116415)

Version 1.74.0 (2023-11-16)
==========================

Language
--------

- [Codify that `std::mem::Discriminant<T>` does not depend on any
  lifetimes in T]
  (rust-lang/rust#104299)
- [Replace `private_in_public` lint with `private_interfaces` and
  `private_bounds` per RFC 2145]
  (rust-lang/rust#113126)
  Read more in
  [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html).
- [Allow explicit `#[repr(Rust)]`]
  (rust-lang/rust#114201)
- [closure field capturing: don't depend on alignment of packed fields]
  (rust-lang/rust#115315)
- [Enable MIR-based drop-tracking for `async` blocks]
  (rust-lang/rust#107421)

Compiler
--------

- [stabilize combining +bundle and +whole-archive link modifiers]
  (rust-lang/rust#113301)
- [Stabilize `PATH` option for `--print KIND=PATH`]
  (rust-lang/rust#114183)
- [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`]
  (rust-lang/rust#115644)
- [Promote loongarch64-unknown-none* to Tier 2]
  (rust-lang/rust#115368)
- [Add `i686-pc-windows-gnullvm` as a tier 3 target]
  (rust-lang/rust#115687)

Libraries
---------

- [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err]
  (rust-lang/rust#98704)
- [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`]
  (rust-lang/rust#111278)
- [impl Step for IP addresses]
  (rust-lang/rust#113748)
- [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`]
  (rust-lang/rust#114041)
- [`impl TryFrom<char> for u16`]
  (rust-lang/rust#114065)
- [Stabilize `io_error_other` feature]
  (rust-lang/rust#115453)
- [Stabilize the `Saturating` type]
  (rust-lang/rust#115477)
- [Stabilize const_transmute_copy]
  (rust-lang/rust#115520)

Stabilized APIs
---------------

- [`core::num::Saturating`]
  (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html)
- [`impl From<io::Stdout> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`std::ffi::OsString::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsString::into_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes)
- [`std::ffi::OsStr::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsStr::as_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes)
- [`std::io::Error::other`]
  (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other)
- [`impl TryFrom<char> for u16`]
  (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16)
- [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

These APIs are now stable in const contexts:

- [`core::mem::transmute_copy`]
  (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html)
- [`str::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii)
- [`[u8]::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii)

Cargo
-----

- [fix: Set MSRV for internal packages]
  (rust-lang/cargo#12381)
- [config: merge lists in precedence order]
  (rust-lang/cargo#12515)
- [fix(update): Clarify meaning of --aggressive as --recursive]
  (rust-lang/cargo#12544)
- [fix(update): Make `-p` more convenient by being positional]
  (rust-lang/cargo#12545)
- [feat(help): Add styling to help output ]
  (rust-lang/cargo#12578)
- [feat(pkgid): Allow incomplete versions when unambigious]
  (rust-lang/cargo#12614)
- [feat: stabilize credential-process and registry-auth]
  (rust-lang/cargo#12649)
- [feat(cli): Add '-n' to dry-run]
  (rust-lang/cargo#12660)
- [Add support for `target.'cfg(..)'.linker`]
  (rust-lang/cargo#12535)
- [Stabilize `--keep-going`]
  (rust-lang/cargo#12568)
- [feat: Stabilize lints]
  (rust-lang/cargo#12648)

Rustdoc
-------

- [Add warning block support in rustdoc]
  (rust-lang/rust#106561)
- [Accept additional user-defined syntax classes in fenced code blocks]
  (rust-lang/rust#110800)
- [rustdoc-search: add support for type parameters]
  (rust-lang/rust#112725)
- [rustdoc: show inner enum and struct in type definition for concrete type]
  (rust-lang/rust#114855)

Compatibility Notes
-------------------

- [Raise minimum supported Apple OS versions]
  (rust-lang/rust#104385)
- [make Cell::swap panic if the Cells partially overlap]
  (rust-lang/rust#114795)
- [Reject invalid crate names in `--extern`]
  (rust-lang/rust#116001)
- [Don't resolve generic impls that may be shadowed by dyn built-in impls]
  (rust-lang/rust#114941)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

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