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

add colors to CLI commands #1231

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JulesDT
Copy link

@JulesDT JulesDT commented Sep 22, 2020

Hello

This PR adds an easy way to add colors to commands when they show up in the terminal.

I personally would use this because amongst my commands, some are more important and having a color code would be helpful and seemed easier / less confusing than generating "categories" or groups in the template.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2020

CLA assistant check
All committers have signed the CLA.

@JulesDT
Copy link
Author

JulesDT commented Sep 22, 2020

I think this answers #59 but somehow the issue was closed, I'm not sure why

@JulesDT JulesDT force-pushed the julesdt/add-colors-in-commands branch from 7095182 to 6291a09 Compare September 22, 2020 00:23
@jpmcb jpmcb added this to the 2.0.0 milestone Oct 18, 2020
@jpmcb
Copy link
Collaborator

jpmcb commented Oct 18, 2020

Thanks for this! Very cool feature!!

Can you provide a short snipped of how a user might use this?

Thanks much for adding tests. I'll give this a test and we should be able to get it in 2.0

@JulesDT
Copy link
Author

JulesDT commented Oct 19, 2020

Hello,

I currently have 2 use cases for this.

The first one would be to put an emphasis on a few commands in my CLI. I have a lot of available CMDs, and 3 of them are groups that regroups the rest of the commands (One group for collection of data, one group analysis of the data and the last group for modification of the data). So I thought it'd be nice to put some color to make them easier to see in the list of commands.

The second use case was to put a color code on some commands. For example, put in red the commands that might modify data or directly affect production, whereas we can put in green commands that would only read data. Making sure that one can easily see that what they're choosing is potentially a dangerous action.

@jpmcb
Copy link
Collaborator

jpmcb commented Oct 19, 2020

Very cool! Can you provide a snippet of code using this new API? That will help me test.

@JulesDT
Copy link
Author

JulesDT commented Oct 19, 2020

If you do a simple cobra init and add this snippet as a file under cmd/ this should show you two commands. delete should be in red and read in green. (Tested on MacOS Catalina 10.15.7)

package cmd

import (
	"fmt"

	"github.com/spf13/cobra"
)

func init() {
	rootCmd.AddCommand(deleteCmd)
	rootCmd.AddCommand(readCmd)
}

var deleteCmd = &cobra.Command{
	Use:   "delete",
	Short: "Delete hosts in production",
	Long:  `Delete hosts in production`,
	Color: cobra.ColorRed,
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("Write CMD")
	},
}

var readCmd = &cobra.Command{
	Use:   "read",
	Short: "read hosts in production",
	Long:  `read hosts in production`,
	Color: cobra.ColorGreen,
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("read CMD")
	},
}

@marckhouzam
Copy link
Collaborator

After your exchange, I started getting curious about this change.

@jpmcb I hope you don't mind, but I tried it out with Helm which is a larger test.

@JulesDT This is really cool! I noticed a couple of things:

  • the name padding isn't working with colours, for example, if I colour the completion command:
$ helm help
[..]
Available Commands:
  2to3            migrate and cleanup Helm v2 configuration and releases in-place to Helm v3
  completion generate autocompletions script for the specified shell
  completion2and3 generate autocompletions script that will work for both helm2 and helm3 together
  conftest        Test Helm Charts
  create          create a new chart with the given name

Notice that the indentation is wrong for completion which is coloured.

Also, the light colours don't seem to work in my case. Maybe it is my terminal?
Which brings the question: how portable is this?

Could this be applied to flags?

@JulesDT
Copy link
Author

JulesDT commented Oct 19, 2020

Thanks for testing this! I indeed made a mistake when putting the colors, the sequence starting at ColorDarkGray is shifted by a few digits. I addressed it in my new commit.

Regarding helm I think the issue is that the padding was badly applied indeed. The unseen characters \033[31m and \033[0m for example were counted as respectively 5 and 4 characters even though they would not show up. So I augmented the padding by the length difference between the ColoredName and the Name. This seems to work for helm now. I added a test case to verify that when using colors, there's an additional padding to take this side effect into account.

Let me know if you see anything else that is wrong!

@marckhouzam
Copy link
Collaborator

@JulesDT It looks great now. Thanks.

But I cannot figure out what happens if the terminal does not support colours? Will the help/usage text be broken?

This is important because if a program using Cobra wants to add colours, can it do it blindly or does it have to support some option to allow users to disable colours, in case their terminal does not support them?
In fact, how would a program allow its user to optionally turn off the colours? Should Cobra provide an easy way to disable colours?

@JulesDT
Copy link
Author

JulesDT commented Oct 20, 2020

That's a good call. I just wasn't able to disable colors in any of the shells I've used. I've tried iterm2, the default terminal, bash, sh in macOS, as well as shells on a linux machine, all were supporting colors, even when setting TERM to xterm-mono or xterm-old.

If you have a way for me to easily disable colors and see how it behaves I'd be happy to push a fix if needed. Detecting if colors are disabled would probably be the best solution, if this can't be done, I guess a config variable that disables colors could work?

I'm tempted to say that people building their own CLI tools will know where it'll run and can enable or disable colors depending on their need, the question arises for global CLI tools (such as helm) where I don't have strong opinion about who should allow to easily disable colors, either the project or Cobra.

@marckhouzam
Copy link
Collaborator

That's a good call. I just wasn't able to disable colors in any of the shells I've used. I've tried iterm2, the default terminal, bash, sh in macOS, as well as shells on a linux machine, all were supporting colors, even when setting TERM to xterm-mono or xterm-old.

I tried as well without success.

... the question arises for global CLI tools (such as helm) where I don't have strong opinion about who should allow to easily disable colors, either the project or Cobra.

With the current proposed implementation, how do you see a program such as helm telling Cobra to disable colours?
I think it would be nice to provide some easy way to do it.

@JulesDT
Copy link
Author

JulesDT commented Oct 25, 2020

Well, if helm doesn't want to provide Colors because they're afraid of incompatibilities, it's easy for them to simply not use that feature from Cobra. Then with the current implementation is basically follows the same behavior as before.

Do you have an alternative idea in mind? I'm happy to implement a config or a parameter or anything that would allow to disable coloring but I'm not sure how such a feature if it existed should be used.

command.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Collaborator

Sorry @JulesDT I was pretty vague in my last comment.

What I'm imagining is that a program would make use of colors but allow a user to disable them (or vice versa where users would be given the option to enable colors). This could be through a global flag, or some configuration option (that's a detail for the program to choose).

So the question becomes, how would the program easily tell Cobra to turn colors on or off in the same compiled binary?

Thinking about it now, I guess the program could have its own function:

func getColor(color TerminalColor) TerminalColor {
   if globalOptionNoColor {
      return null
   }
    return color
}

But I was wondering if it would be easier for Cobra to provide an option so that ColoredName() could do this disabling.

@JulesDT
Copy link
Author

JulesDT commented Oct 26, 2020

I see. Yeah I wouldn't disagree with giving the user this possibility. In term of design or user experience, I'm not sure what would be the preferred way for Cobra, or what is usually used for such options here.

What I guess a program could do indeed is set a global flag such that if this flag is set ColoredNamed would return the normal name or not. What I suggest for this is the following, but I don't love this design so I'm happy to be told a different way you'd prefer.
On the command itself, add a DisableColors bool parameter.
And then for the ColoredName method, we can do:

func (c *Command) isColoringDisabled() bool {
	if c.DisableColors {
		return c.DisableColors
	}
	if c.parent != nil {
		return c.parent.isColoringDisabled()
	}
	return false
}

func (c *Command) ColoredName() string {

	if c.Color != 0 && !c.isColoringDisabled() {
		return fmt.Sprintf("\033[%dm%s\033[0m", c.Color, c.Name())
	}
	return c.Name()
}

Then what one could do is add a persistent flag on root to disable colors:

rootCmd.PersistentFlags().BoolVarP(&rootCmd.DisableColors, "disable-colors", "d", false, "disable colors in the terminal")

What do you think @marckhouzam ?

@JulesDT
Copy link
Author

JulesDT commented Dec 1, 2020

@marckhouzam or @jpmcb , any thoughts on this?

@catthehacker
Copy link

I suggest following the https://no-color.org/ standard.

@marckhouzam
Copy link
Collaborator

I suggest following the https://no-color.org/ standard.

I think respecting a $NO_COLOR environment variable is a good idea.
Is that sufficient though? Using NO_COLOR will disable colours for many programs. Does Cobra, as a library to other programs, need to provide a way to disable colours only for the program itself?

@marckhouzam
Copy link
Collaborator

@JulesDT Sorry for the delay.

I like your idea of having a DisableColors bool parameter. I don't think we want to allow to disable colours on a per-command basis, so such a boolean parameter could be applicable to the root command only. So then you would do something simple like:

func (c *Command) ColoredName() string {
	if c.Color != 0 && !c.Root().DisableColors {
		return fmt.Sprintf("\033[%dm%s\033[0m", c.Color, c.Name())
	}
	return c.Name()
}

Then, a program could disable colours using a global flag using something like:

	rootCmd.PersistentFlags().BoolVar(&rootCmd.DisableColors, "no-colors", false, "disable colors")

These are just ideas but please be aware I have no final say on this.

@catthehacker
Copy link

I suggest following the no-color.org standard.

I think respecting a $NO_COLOR environment variable is a good idea.
Is that sufficient though? Using NO_COLOR will disable colours for many programs. Does Cobra, as a library to other programs, need to provide a way to disable colours only for the program itself?

That is the idea behind NO_COLOR environment variable. It's supposed to disable ALL colours for every program.
I think that if Cobra is to support colours it should respect NO_COLOR for itself and for any program that is using Cobra.
If someone would like to not have colours in Cobra only, they could use the already proposed option of --no-colors. Just my 2c, I would prefer it named --no-color

@marckhouzam
Copy link
Collaborator

I think that if Cobra is to support colours it should respect NO_COLOR for itself and for any program that is using Cobra.
If someone would like to not have colours in Cobra only, they could use the already proposed option of --no-colors. Just my 2c, I would prefer it named --no-color

Sounds good to me.

@umarcor
Copy link
Contributor

umarcor commented Dec 14, 2020

Some thoughts:

  • Using viper, the option can be set through a config file, envvars or a flag. Supporting one or several of those options should be up to users of cobra as a library. Hence, I think the discussion above about how to make this a single "global" option (or an option of the root) is enough with regard to the codebase. Then, a usage example can be provided for showcasing how to use it along with viper. Optionally, the template of the cobra cli might be enhanced.

  • Many tools try to automatically disable/enable the colour depending on the terminal being a TTY or not. On top of that, options are provided for forcing colouring and/or not colouring. That is, supporting USE_COLOR or FORCE_COLOR as a complement to NO_COLOR. Some tools provide other custom variables/options:

  • There is an issue with GitHub Actions compared to other CI services. Typically, "detection" in CI works and logs are coloured, but that's not the case in Actions: Not a tty actions/runner#241. Hence, instead of trying to detect whether to enable colours, I think that users should set a default and allow envvars (viper).

    • A workaround in Actions is to run scripts inside a container, which does provide a TTY.

@JulesDT
Copy link
Author

JulesDT commented Dec 23, 2020

@umarcor correct me if I'm wrong, but I think the problem around using viper is that when the help menu is being shown to the user (where the colors are used basically), the config file hasn't been parsed yet.
The parsing happens with cobra.OnInitialize(initConfig) which only happens when the command is about to run basically.

Would the --no-color parameter and the NO_COLOR env variable be enough?

@jpmcb
Copy link
Collaborator

jpmcb commented Jan 22, 2021

I would also like to see a test case around the --no-color and NO_COLOR options if possible.

Checking in on this one, where are we at with this PR? Thanks much for the work on this one :D

@JulesDT
Copy link
Author

JulesDT commented Jan 22, 2021

Hello @jpmcb ! Good catch on the tests, I added a test related to the NO_COLOR env variable and the DisableColors parameter. Let me know if you see anything else that is missing.

On my end, I don't think anything is missing and I think all concerns above have been considered.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

I've tested the latest version of this PR with helm where I added a bunch of sub-commands with colours. It all looks great, both the colours and the ability to disable them. Thanks @JulesDT.

Screen Shot 2021-01-23 at 10 29 46 PM

Maybe it would be nice to add little mention of this feature in the docs? And if so, including how to turn it off. What do you think @jpmcb .

@catthehacker
Copy link

catthehacker commented Jan 30, 2021

So I've just looked at the code out of curiosity.
You should check for Linux terminals that don't support colouring ($TERM = dumb)
But also, was that tested on Windows? With which versions it will be compatible?
Did you consider using external dependency for colour output and/or terminal detection? (e.g.: https://github.com/mattn/go-colorable / https://github.com/mattn/go-isatty / https://github.com/fatih/color)

@umarcor
Copy link
Contributor

umarcor commented Jan 30, 2021

@umarcor correct me if I'm wrong, but I think the problem around using viper is that when the help menu is being shown to the user (where the colors are used basically), the config file hasn't been parsed yet.
The parsing happens with cobra.OnInitialize(initConfig) which only happens when the command is about to run basically.

@JulesDT. You are correct. If the command does not have a Run field, initConfig is not used.

@marckhouzam
Copy link
Collaborator

What shall we do with this colour PR? Reading the comment from @catthehacker, I don't know if this change will work on Windows, @JulesDT do you have the ability to try it? Maybe using an external package would be a good idea?

@catthehacker
Copy link

I don't know if this change will work on Windows

@marckhouzam Yes and no. It will work for people using Windows Terminal app but it will not work in default terminal.
image

@JulesDT
Copy link
Author

JulesDT commented Mar 5, 2021

Thanks for the screenshots and tests @catthehacker. If we are okay with using an external dependency, I can try to make the switch, I don't really have a Windows available for testing it.

Do we have any preference around which one to use?

@catthehacker
Copy link

catthehacker commented Mar 5, 2021

Just wanted to add that I pointed to those packages as reference, I'm not well versed in using any of them and there might be better/more suitable libraries to use (or not using external deps at all).

Also I can test anything on current (Win10 2009), preview (dev builds) and previous (2004, 1909, 1903, Win8.1, Win8, Win7, etc.) Windows versions for you if you need.

@github-actions
Copy link

github-actions bot commented May 5, 2021

This PR is being marked as stale due to a long period of inactivity

@stmcginnis
Copy link

I like the idea of this, but a little worried since it has such a broad impact on everyone using the library.

The biggest area of concern for me is Windows. As I understand it, right now the most common terminals are the legacy cmd.exe, powershell.exe, and the newer Windows Terminal. Of the three, only Windows Terminal will display colors by default. This will show what would appear to be garbage characters with the first two.

There are the options added to disable colorization, but for a Windows user running the default shell, it puts an extra burden on them to have to set the environment variable or add the argument to disable it.

I think it would be good to add some detection and automatically disable colorization if we can tell we are running somewhere that doesn't support it. Here is one way I've accomplished this in another project:

if runtime.GOOS == "windows" {
	// The newer Windows Terminal supports color, cmd and powershell do
	// not. Currently the only way to tell if you are running in WinTerm is
	// by the presence of a "WT_SESSION" environment variable.
	result = os.Getenv("WT_SESSION") != ""
}

There may be a better way to determine that, but of course the Windows terminals don't support the same variables and methods that Linux and macOS terminals do.

@umarcor
Copy link
Contributor

umarcor commented Nov 2, 2021

@stmcginnis, note that Cygwin, MSYS2, Git for Windows, etc. are available on Windows. Those support ANSI colours.
See the following discussion: tartley/colorama#306. Hence, you are effectively proposing a reimplementation of colorama inside cobra. I believe it is desirable to offload that complexity to some other "golang color CLI" package, such as the ones suggested by @catthehacker above.

@stmcginnis
Copy link

I believe it is desirable to offload that complexity to some other "golang color CLI" package

Yep, I agree. Based on that, and the complexity of correctly detecting terminal capabilities, and the fact that this impacts all users of Cobra (a lot!) then I am -1 on this proposal. I don't think cobra should be handling color formatting and it should be left to another library to handle it.

@umarcor
Copy link
Contributor

umarcor commented Nov 2, 2021

While I agree, I believe it would be interesting to provide a code example about how to achieve it. Maybe cobra can provide some hooks (an API) for users to plug their own (or third-party) colouring package.

@JulesDT
Copy link
Author

JulesDT commented Nov 2, 2021

@stmcginnis considering what you said, if we relied on https://github.com/fatih/color in ColoredName would that work for you? I'm happy to make that change, I still think Cobra would benefit a lot from allowing colors because this makes it so much easier to use in some tools.

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

This PR is being marked as stale due to a long period of inactivity

@ivanpirog
Copy link

ColoredCobra: https://github.com/ivanpirog/coloredcobra

ColoredCobra is a small library that allows you to colorize the text output of the Cobra library, making the console output look better.

@marckhouzam marckhouzam removed this from the 2.0.0 milestone Oct 1, 2022
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

8 participants