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

Beautify help messages #151

Merged
merged 3 commits into from
Jun 5, 2021
Merged

Beautify help messages #151

merged 3 commits into from
Jun 5, 2021

Conversation

QuarticCat
Copy link
Contributor

  1. Add wrap_help feature to clap
  2. Enable AppSettings::ColoredHelp

Before:

image

After:

image

Copy link
Owner

@ducaale ducaale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help looks much nicer now, thanks.

src/cli.rs Outdated Show resolved Hide resolved
@blyxxyz
Copy link
Collaborator

blyxxyz commented Jun 4, 2021

Very pretty!

Can you check if generate.sh still works? I'm worried help2man will get confused.

@QuarticCat
Copy link
Contributor Author

@blyxxyz It still works. By default AppSettings::ColorAuto is enabled.

@ducaale
Copy link
Owner

ducaale commented Jun 4, 2021

Generated manpages seem to be okay. This is because coloring gets disabled when output is being redirected.

However, It seems that NO_COLOR doesn't disable colored help.

@QuarticCat
Copy link
Contributor Author

@ducaale That's weird. Because clap also uses termcolor which respects NO_COLOR, and NO_COLOR take effect on my small cli project. I'm trying to figure out what went wrong.

@QuarticCat QuarticCat closed this Jun 4, 2021
@QuarticCat QuarticCat reopened this Jun 4, 2021
@QuarticCat
Copy link
Contributor Author

OK, I found why. In clap 2.3, it implements color output through ansi_term instead of termcolor, which does not support NO_COLOR (see ogham/rust-ansi-term#68) and has no other global toggle (see ogham/rust-ansi-term#10).

I'm wondering if there is a way to inject AppSettings::ColorNever but I have to go to sleep now.

@QuarticCat
Copy link
Contributor Author

This workaround is a bit ugly for me... Anyway, I will see your opinions tomorrow.

src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated
@@ -296,6 +303,11 @@ impl Cli {
match Self::from_iter_safe(iter) {
Ok(cli) => cli,
Err(err) if err.kind == ErrorKind::HelpDisplayed => {
let mut app = if env::var("NO_COLOR").is_ok() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After experimenting a bit I think this is the best way out. It's possible to look at termcolor's detection, but NO_COLOR is really the only piece that's missing.

One nitpick, so unicode won't matter:

Suggested change
let mut app = if env::var("NO_COLOR").is_ok() {
let mut app = if env::var_os("NO_COLOR").is_some() {

@QuarticCat
Copy link
Contributor Author

QuarticCat commented Jun 5, 2021

@blyxxyz @ducaale Done. But I find a wrong output case:

image

The last case seems not handled by clap.

@blyxxyz
Copy link
Collaborator

blyxxyz commented Jun 5, 2021

Ah, that's unfortunate. It already did that, so you can ignore it for this PR if you want, and I'll look at it later. The problem seems to be that Error::with_description uses automatic color detection.

Can you target develop instead of master?

@QuarticCat QuarticCat changed the base branch from master to develop June 5, 2021 11:18
@ducaale ducaale merged commit 95d6fc7 into ducaale:develop Jun 5, 2021
@blyxxyz
Copy link
Collaborator

blyxxyz commented Jun 5, 2021

One way this does affect generate.sh is that the terminal size now determines when long lines in the help output are split, and that's visible in xh.1.

But that doesn't affect the rendered man page so it's fine.

@blyxxyz blyxxyz mentioned this pull request Jun 5, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants