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

refactor: Upgrade to clap v4 #2327

Merged
merged 4 commits into from
Oct 30, 2022
Merged

refactor: Upgrade to clap v4 #2327

merged 4 commits into from
Oct 30, 2022

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 28, 2022

This changes the format of help output

  • Default features were enabled as we bat doesn't seem to disable any and it is easier to keep up with new default features like help, usage, and error-context
  • Help styling was removed because styling can't be applied to help_template atm which is required for showing cache command last
  • Title Case is used instead of ALL CAPS
  • "Command" is used instead of "Subcommand"
  • Name/version isn't in --help anymore

@sharkdp
Copy link
Owner

sharkdp commented Oct 3, 2022

Thank you very much!

It's probably good to mention that this changes the layout/look of the help text quite dramatically:

  • Something seems wrong with the "Usage" line. This needs to be fixed.
  • No colorization by default. I think I'm okay with this.
  • The section headings are not ALL-CAPS anymore. Instead, they are underlined. I personally preferred the previous option, but I'm open to a change.
  • The "Commands" section (previously: Subcommands) is now in a very prominent place at the top. That might be a good idea in general, but not for bat. The cache subcommand will only be used in special circumstances.
  • The "bat 0.22.1 (…)" part (name + version) is missing. This is a new default in clap. I'm okay with this. We have bat --version.
  • The "Use '--help' instead of '-h' to see a more detailed version of the help text." and "Note: bat -h prints a short and concise overview while bat --help gives all details." parts are missing. I would like to have them back.

image

compare:
image

@epage
Copy link
Contributor Author

epage commented Oct 3, 2022

The "Use '--help' instead of '-h' to see a more detailed version of the help text." and "Note: bat -h prints a short and concise overview while bat --help gives all details." parts are missing. I would like to have them back.

Before doing so, I wanted to double check that this request is with the understanding that --help's description includes this kind of context as well and that you still want the other explanations on top of that.

@sharkdp
Copy link
Owner

sharkdp commented Oct 3, 2022

The "Use '--help' instead of '-h' to see a more detailed version of the help text." and "Note: bat -h prints a short and concise overview while bat --help gives all details." parts are missing. I would like to have them back.

Before doing so, I wanted to double check that this request is with the understanding that --help's description includes this kind of context as well and that you still want the other explanations on top of that.

Not sure I understand correctly. I think that "Use '--help' instead of '-h' to see a more detailed version of the help text." line at the very end of the -h and --help text was very helpful, because we hide a lot of options in the -h text and use much shorter descriptions. I'm okay with removing the other part (which was positioned in the top description of the -h text).

@epage
Copy link
Contributor Author

epage commented Oct 3, 2022

Clap's auto-generated --help flag now includes a hint about this as well, inspired by bat:
image
(use --help for more details which showed up because you used -h)

@sharkdp
Copy link
Owner

sharkdp commented Oct 3, 2022

Clap's auto-generated --help flag now includes a hint about this as well, inspired by bat:

Ohhh, I completely missed that. In that case.. let's hope that others don't miss it as well. And skip those additional explanations.

Thank you for the updates! I would like to hear the opinions of the other maintainers before merging this. For reference, the new output looks like this:

image

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

First of all, thanks a lot for working on this.

I do however wonder if want clap v4 enough to sacrifice readability of help output (see separate comment).

",
)
// Doesn't play well with `help_template`
.disable_colored_help(true)
Copy link
Collaborator

@Enselic Enselic Oct 4, 2022

Choose a reason for hiding this comment

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

I wouldn't say I feel very strongly about this, but I do think the lack of colors in the help output is quite a significant regression, especially given that bat is all about colored output.

What would be the downsides of bat waiting with upgrading to v4 until v4 supports colors like v3?

From a development point of view it feels very nice to use the latest of everything, but I do think we also need to look at this from a user perspective. Is there any big benefit from a users point of view by upgrading to v4 already now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, which milestone do you consider a blocker:

  • Being able to provide a help_template that can be stylized like the auto-generated content (the reason Use disable_colored_help)
  • Customizing clap's help styling / theme

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern is basically that headers stopped being orange and args stopped being yellow. Not sure what it takes to get those colors back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clap v4 moved away from yellow section headers and green args. See the announcement for details. They were replaced by bold and underline.

However, that is not why disable_colored_help is set. clap v4 also moved the subcommands to the top which is great for most applications where the focus is on the subcommands but that doesn't work well for bat where the subcommand isn't a focus. Bat now has to provide its own help_template to override the help ordering but this means there are "Arguments", "Options", and "Commands" headers that cannot be styled at this time but other headers are styled. To make things consistent, I've turned off all styling.

So if you want theming from clap (clap-rs/clap#3234), you first need help_template styling (clap-rs/clap#1433). My initial focus is on help_template styling. After that I'll focus on theming. The main issue around theming is having a stable API for specifying ansi codes. I've created anstyle for this but need more experience reports / feedback on it before I feel its ready to go 1.0. My hope is the help_tempalate work will give me experience with it internal to clap to help.

If people want to help towards check, I'd recommend checking out anstyle and providing any feedback on it as a general crate for other crates to have theming support in their API without being tied to termcolor or one of the other crates.

Copy link
Owner

@sharkdp sharkdp Oct 30, 2022

Choose a reason for hiding this comment

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

To resolve this, I made the following changes:

  • I removed the custom template, which re-enables bold/underline styling. Which takes some getting used-to (if you are used to the colorized output), but is IMO also a very reasonable styling alternative that is easy to read.
  • I set the bat cache subcommand to hidden. This removes the whole "command" section from both -h and --help text.
  • I added a new .after_help(…) text that let's users know that there is a bat cache subcommand.

I would really like to integrate the changes here, mostly because I don't want to do this maintenance work later on if we want to upgrade clap for other reasons. I think the "regression" is not too bad. But as I mentioned in an email conversation with @epage as well, I liked the previous "theme" better as well. @Enselic I hope that is okay for you.

@epage: Thank you very much for your patience and this PR!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable to me, nice job 👍
(Long term we might be able to bring back the colors)

@@ -77,10 +77,9 @@ default-features = false
features = ["parsing"]

[dependencies.clap]
version = "3.2.20"
version = "4.0.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to bump to latest already now (4.0.10 as of now)

Copy link
Owner

Choose a reason for hiding this comment

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

Done. (Note that the precise version is specified in the lockfile, not here)

@ofek
Copy link

ofek commented Oct 26, 2022

This looks great!

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.

6 participants