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

Added new alias for 'decoration=always' #1141

Merged
merged 9 commits into from
Sep 14, 2020
Merged

Conversation

alexanderkarlis
Copy link
Contributor

This PR addresses #1108 .

Hey all, first time making a PR for a rust repo and still pretty new to it.

Think I addressed the feature here for adding an alias for --decoration=always with the -D arg. The -D commands works the same as if --decoration=always or if running on a terminal that can support decorations (e.g- --decoration=auto)

Let me know. Thanks!

@sharkdp
Copy link
Owner

sharkdp commented Aug 30, 2020

Thank you very much. The implementation looks good.

My only concern is the letter for the flag. I would actually like to combine --decorations=always with --color=always into a single flag. I was thinking about -f for "force color and decorations", which would still be free (it's also not used in cat).

What do you think?

Also, @eth-p: how do you feel about introducing a new short option for this? I tend to use --color=always a lot, but my use of bat is maybe not the best indicator.

@mouse07410
Copy link

I think it's a great idea.

@eth-p
Copy link
Collaborator

eth-p commented Aug 31, 2020

I think it would be a good idea. I've found myself wanting to pipe bat output into other programs before, and it's a bit inconvenient to have to pass both by full name.

On top of that, it would also be a nice analogy to the -p flag (albeit doing the exact opposite thing).

@alexanderkarlis
Copy link
Contributor Author

I think that's a good tip. And a seemingly useful alias combo. I will give it another go with both flags.

.arg(
Arg::with_name("always-decorations")
.short("f")
.alias("always-decor")
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see much value in this. For me, long options are useful for scripts, due to the self-documenting character. Short options are useful when I have to type it in the console. An abbreviated long option would be for what?

Arg::with_name("always-decorations")
.short("f")
.alias("always-decor")
.overrides_with("always-decorations")
Copy link
Owner

Choose a reason for hiding this comment

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

Should it also "conflict with" --color=… and --decorations=…?

.short("f")
.alias("always-decor")
.overrides_with("always-decorations")
.hidden(true)
Copy link
Owner

Choose a reason for hiding this comment

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

Hiding from short -h text is fine, but I think this should appear in the long --help text.

.overrides_with("always-decorations")
.hidden(true)
.hidden_short_help(true)
.help("Alias for '--decorations=always --color=always'")
Copy link
Owner

Choose a reason for hiding this comment

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

Can we please use a proper sentence here like for the rest of the command line options? Maybe we could also add:

"This is useful if the output of bat is piped to another program, but you want to keep the colorization/decorations."

@@ -257,6 +257,15 @@ pub fn build_app(interactive_output: bool) -> ClapApp<'static, 'static> {
an interactive terminal is detected. Possible values: *auto*, never, always.",
),
)
.arg(
Arg::with_name("always-decorations")
Copy link
Owner

Choose a reason for hiding this comment

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

With the extended functionality, I would rename this to --force-colorization maybe?

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2020

Could you please also update the man page in assets/manual/bat.1.in?

Also, could you please add an entry to the "unreleased" section in CHANGELOG.md? The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or the PR and user is your username.

@sharkdp
Copy link
Owner

sharkdp commented Sep 14, 2020

Thank you very much for the updates!

@sharkdp sharkdp merged commit 5df449b into sharkdp:master Sep 14, 2020
@sharkdp sharkdp mentioned this pull request Sep 14, 2020
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.

None yet

4 participants