Add support for NO_COLOR and CLICOLOR (#428) #432

dupgit · 2020-03-17T13:56:35Z

Takes care of NO_COLOR, CLICOLOR and CLICOLOR_FORCE environment variables. NO_COLOR takes precedence over CLICOLOR_FORCE itself taking precedence over CLICOLOR itself taking precedence over --color command line option.

When none of NO_COLOR, CLICOLOR and CLICOLOR_FORCE environment variables
are defined the old behavior still works and command line options are taken into account.

degremont

Thanks for the patch. Please see below few comments.

It could be good to also add tests in CLIDisplay I think and also add somewhere in some docs that these variables are now supported.

degremont · 2020-03-18T09:05:14Z

lib/ClusterShell/CLI/Display.py

+        """Tests NO_COLOR environment variable to determine if color
+        must be muted on output."""
+
+        if os.getenv('NO_COLOR') is not None:


prefer

return 'NO_COLOR' in os.environ

And this could be directly use above maybe, no need for a function?

degremont · 2020-03-18T09:08:58Z

lib/ClusterShell/CLI/Display.py

+
+        cli_color = os.getenv("CLICOLOR")
+
+        if cli_color is None:


Looks like:

if cli_color is None: return None elif cli_color != "0": return sys.stdout.isatty() else: return False

degremont

Overall ok, just minor style change

degremont · 2020-03-19T09:43:43Z

lib/ClusterShell/CLI/Display.py

        elif cli_color != "0":
-            # CLICOLOR should be used but stdout is not a tty.
-            return False
+            # CLICOLOR are ment to be used if stdout is a tty


not sure what 'ment' means?

degremont · 2020-03-19T09:44:23Z

lib/ClusterShell/CLI/Display.py

-            # CLICOLOR should be used but stdout is not a tty.
-            return False
+            # CLICOLOR are ment to be used if stdout is a tty
+            return sys.stdout.isatty()
        elif cli_color == "0":


prefer a simple else here

degremont · 2020-03-19T09:55:20Z

Could be good to change the commit message.

Squash all your commits in one
Add a small subject line as first commit line, starting with Defaults:
Add (#428) at the end
Add Closes #428 in the commit message
Add a Signed-Off line

See how other commit messages look like.

degremont · 2020-03-20T08:54:52Z

Looks good to me. I was wondering if the doc text should not be written using a dedicated section for Environment variables, or something similar.

I will let @thiell validate this part.

thiell · 2020-03-27T21:26:22Z

Hi @dupgit,
Thanks much for this PR. We think the command line option --color should always have precedence over these environment variables. It makes more sense especially if a user wants to be sure to disable any colors with --color=never. But actually, in all cases when --color is specified by the user, we think that environment variables should just be ignored (even for --color=auto).
Could you please make this last change?
Again, thanks much!

thiell · 2020-03-27T21:28:10Z

doc/txt/clubak.txt

@@ -57,7 +57,7 @@ OPTIONS
               node / line content separator string (default: `:`)
 -F, --fast     faster but memory hungry mode (preload all messages per node)
 -T, --tree     message tree trace mode; switch to enable ``ClusterShell.MsgTree`` trace mode, all keys/nodes being kept for each message element of the tree, thus allowing special output gathering
--color=WHENCOLOR   whether to use ANSI colors to surround node or nodeset prefix/header with escape sequences to display them in color on the terminal. *WHENCOLOR* is ``never``, ``always`` or ``auto`` (which use color if standard output refers to a terminal). Color is set to [34m (blue foreground text) and cannot be modified.
+--color=WHENCOLOR   ``clush`` is compliant with NO_COLOR, CLICOLOR and CLICOLOR_FORCE environment variables. NO_COLOR takes precedence over CLICOLOR_FORCE which takes precedence over CLICOLOR. If these environment variables are not used then ``--color`` tells whether to use ANSI colors to surround node or nodeset prefix/header with escape sequences to display them in color on the terminal. *WHENCOLOR* is ``never``, ``always`` or ``auto`` (which use color if standard output refers to a terminal). Color is set to [34m (blue foreground text) and cannot be modified.


--color should have precedence over environment variables

dupgit · 2020-06-10T20:53:09Z

Hi,
I squash rebased this work so there is only one commit for the whole issue. @thiell please let me know if this is now correct for you now ?
Thanks.

thiell · 2020-07-17T04:20:59Z

Hi @dupgit,

Thanks for the change and sorry for the delay. While testing your patch, I found an issue that annoys me a bit. If color is defined in clush.conf, the color environment variables don't seem to be taken into account anymore, even if color: auto in clush.conf.

We could fix the patch so that if color: auto is defined, environment variables are still considered. They probably should not if color: always or color: never is set.

Or perhaps even better, I'm wondering if we should offer more choices in clush.conf: something like env (use env variables, or otherwise auto), auto, always, never and set env as the new default. Let me know what you think.

dupgit · 2020-07-21T20:46:27Z

Hi @dupgit,

Hi @thiell,

Thanks for the change and sorry for the delay. While testing your patch, I found an issue that annoys me a bit. If color is defined in clush.conf, the color environment variables don't seem to be taken into account anymore, even if color: auto in clush.conf.

Thanks for the rewiew. I thought that options that are set in either command line or configuration file should take precedence over color environment variables.

We could fix the patch so that if color: auto is defined, environment variables are still considered. They probably should not if color: always or color: never is set.

I will fix the proposed patch in that way: environment variable are still taken into account when color:auto or no color option is configured in clush.conf file. Nothing changes for command line: if any parameter is set in the command line environment variables are ignored.

Or perhaps even better, I'm wondering if we should offer more choices in clush.conf: something like env (use env variables, or otherwise auto), auto, always, never and set env as the new default. Let me know what you think.

I think that this will be more confusing to the user and will make the patch more complex. Getting auto taking environment variable automatically into account is simpler to me: if env is defined and no environment variable is defined at all then the expected behavior would be like auto. So lets keep it and add the fact that it can now take environment variable into account.

dupgit · 2021-04-13T12:22:28Z

@thiell @degremont is there anything left here to help merging this patch ?

Adds support for NO_COLOR and CLICOLOR specification standard for ANSI colors in terminals. --color option takes precedence over environment variables. It also takes care of configuration file options. Changes THREE_CHOICES index in order to test things correctly. Closes cea-hpc#428. Signed-off-by: Olivier DELHOMME <[email protected]>

thiell

Thank you, @dupgit, for your contribution!

dupgit mentioned this pull request Mar 17, 2020

CLI_COLOR support #428

Closed

degremont requested changes Mar 18, 2020

View reviewed changes

dupgit requested a review from degremont March 18, 2020 20:01

degremont requested changes Mar 19, 2020

View reviewed changes

degremont changed the title ~~Proposal to issue #428~~ Add support for NO_COLOR and CLICOLOR (#438) Mar 19, 2020

degremont changed the title ~~Add support for NO_COLOR and CLICOLOR (#438)~~ Add support for NO_COLOR and CLICOLOR (#428) Mar 19, 2020

degremont assigned dupgit Mar 19, 2020

degremont requested a review from thiell March 19, 2020 09:52

degremont added the enhancement label Mar 19, 2020

dupgit force-pushed the master branch from 72ca086 to f1f4355 Compare March 19, 2020 12:53

degremont self-requested a review March 20, 2020 08:52

degremont approved these changes Mar 20, 2020

View reviewed changes

thiell requested changes Mar 27, 2020

View reviewed changes

dupgit requested a review from thiell April 12, 2020 15:34

dupgit force-pushed the master branch from d806ec9 to 60475c5 Compare June 10, 2020 20:22

dupgit force-pushed the master branch from 60475c5 to 9e5f8df Compare July 21, 2020 20:58

dupgit requested a review from degremont December 10, 2020 20:58

thiell force-pushed the master branch from 9e5f8df to 149cfdd Compare November 4, 2021 04:10

thiell approved these changes Nov 4, 2021

View reviewed changes

thiell merged commit e83a2e1 into cea-hpc:master Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for NO_COLOR and CLICOLOR (#428) #432

Add support for NO_COLOR and CLICOLOR (#428) #432

Uh oh!

dupgit commented Mar 17, 2020

Uh oh!

degremont left a comment

Uh oh!

degremont Mar 18, 2020

Uh oh!

degremont Mar 18, 2020

Uh oh!

degremont left a comment

Uh oh!

degremont Mar 19, 2020

Uh oh!

degremont Mar 19, 2020

Uh oh!

degremont commented Mar 19, 2020

Uh oh!

degremont commented Mar 20, 2020

Uh oh!

thiell commented Mar 27, 2020

Uh oh!

thiell Mar 27, 2020

Uh oh!

dupgit commented Jun 10, 2020

Uh oh!

thiell commented Jul 17, 2020

Uh oh!

dupgit commented Jul 21, 2020

Uh oh!

dupgit commented Apr 13, 2021

Uh oh!

thiell left a comment

Uh oh!

Uh oh!

Add support for NO_COLOR and CLICOLOR (#428) #432

Add support for NO_COLOR and CLICOLOR (#428) #432

Uh oh!

Conversation

dupgit commented Mar 17, 2020

Uh oh!

degremont left a comment

Choose a reason for hiding this comment

Uh oh!

degremont Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

degremont Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

degremont left a comment

Choose a reason for hiding this comment

Uh oh!

degremont Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

degremont Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

degremont commented Mar 19, 2020

Uh oh!

degremont commented Mar 20, 2020

Uh oh!

thiell commented Mar 27, 2020

Uh oh!

thiell Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

dupgit commented Jun 10, 2020

Uh oh!

thiell commented Jul 17, 2020

Uh oh!

dupgit commented Jul 21, 2020

Uh oh!

dupgit commented Apr 13, 2021

Uh oh!

thiell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!