lib/colors.sh - set NO_COLOR=1 when stdout is not a tty

[meleu]

Colorized output is pretty neat in interactive sessions. But when we want to use pipes or redirection, those ANSI escape codes are pollution.

There's a POSIX-compliant way to check if a script is being executed in an interactive session test -t.

Here's the description from the official specification

-t file_descriptor

True if file descriptor number file_descriptor is open and is associated with a terminal. False if file_descriptor is not a valid file descriptor number, or if file descriptor number file_descriptor is not open, or if it is open but is not associated with a terminal.

I usually use the bash version:

# the 1 here represents the stdout
[[ -t 1 ]] && echo "I want colors!" || echo "NO_COLOR, please"

It would be cool if the colors.sh from the bashly standard library was smart enough to check if the output is going to a terminal or not (pipe/redirection).

I tried adding the [[ -t 1 ]] logic in the print_in_color function, but soon realized that when it's called as a consequence of echo "before $(red this is red) after", the stdout is never a terminal (in other words: [[ -t 1 ]] is always false).

The solution I've found was adding this to my src/initialize.sh:

  [[ -t 1 ]] || export NO_COLOR=1

and it works pretty well.

It also works if we put that line in the top-level scope of colors.sh, which would make the solution available for everyone with no need to tweak the initialize.sh.

What do you think?

[meleu]

Example of expected result:

[pcrockett]

I think I might agree with this, with a small added detail: The default should be exactly what you propose, BUT it should still remain easy for the user to override the default.

Or in other words, when the user fails to specify how they want color to be output, try to be "smart" about it using the -t approach. But when the user DOES specify, just blindly do what they ask (either always outputting color, or never outputting color).

[pcrockett]

Perhaps something like this:

Current implementation:

print_in_color() {
  local color="$1"
  shift
  if [[ -z ${NO_COLOR+x} ]]; then
    printf "$color%b\e[0m\n" "$*"
  else
    printf "%b\n" "$*"
  fi
}

New implementation:

print_color_always() {
  local color="$1"
  shift
  printf "$color%b\e[0m\n" "$*"
}

print_newline() {
  printf "%b\n" "$*"
}

if [ "${NO_COLOR}" == "1" ]; then
  print_in_color() {
    shift
    print_newline "$@"
  }
elif [ "${NO_COLOR}" == "0" ]; then
  print_in_color() {
    print_color_always "$@"
  }
elif [ -t 1 ]; then
  print_in_color() {
    print_color_always "$@"
  }
else
  print_in_color() {
    shift
    print_newline "$@"
  }
fi

[DannyBen]

I am not convinced at all. Here is why:

1. All of bashly's libraries - including colors.sh - are dropped into the user space by design, allowing each user to do with them as they wish.
2. Bashly itself does not make use of any of its own standard libraries by design. The only small exception, is the usage_color configuration - but in this case, it simply calls any color function that you configure - so you can use the bashly colors library, but you are not obligated to do so.
3. If a "heavy" bashly user wants to always have a different set of framework libraries, they can easily create their own, which makes it easy to completely ignore any of these extras that bashly provides if they do not align with the developer's programming style.
4. Adding a configuration that lets users choose how the standard library script behaves, in a program that is already sort of a configuration tool seems like a slight over engineering to me, and as Phil mentioned, such a configuration might be needed.
5. If my memory serves, the NO_COLOR standard is binary in nature - either "yes, show colors" or "no, avoid colors". Now if you add auto detection to the mix, then binary environment variable is insufficient; think about a situation where NO_COLOR=1 but the program detects that it is not in a TTY - what would one user expect? and will it be the same expectation for the other user? The argument for auto detection would be stronger if NO_COLOR would have supported a third state like NO_COLOR=auto - then it would be crystal clear.

That said - if there is a way to slightly (read: "while keeping it simple") modify the bundled colors library in order to make it easier to enable this behavior - I am for it.

[pcrockett]

for what it's worth, point 5 is addressed in my above suggestion. it checks env variables first and does whatever the environment says. if no env variables are specified, then it autodetects.

also, i don't think i'd implement it exactly the way i suggested above -- that was just a quick brain dump. maybe i can find some time in the next weeks / months to submit a PR that abides by the "slight" change requirement.

[pcrockett]

eh, i read the NO_COLOR standard a little more closely and decided that it's not feasible to do this generically for everyone. i think we should leave the color functionality alone.

that said, because i read the spec a little more closely, i have a bugfix for you 😁 #610

though... arguably more of a nitpick than a bugfix.

[pcrockett]

after looking at it in-depth, i have come to the conclusion that script developers should implement their own --color=[always|never|auto] flag if they want things like auto-detection. it's probably not a good idea to put auto-detection in the colors library.

that said, this would be another good "recipe" one could add to the wiki.

[DannyBen]

Yeah, well - printing things in color is quite a basic ability that I was hoping would be at a better "standards" state by now.
Not to diminish the effort that the NO_COLOR guys have done, I think it is incomplete.

I guess we can easily do an alternative colors library that explicitly does NOT follow NO_COLOR, then we can do whatever we want.

[DannyBen]

Here is my proposal:

1. Update the color library slightly to support two standards: the usual NO_COLOR, and our own COLOR=always|never|auto.
2. If COLOR is set, it has priority.
3. In the code below, I have separated the use_color? boolean function, so that anyone can update it separately if they see fit.

print_in_color() {
  local color="$1"
  shift

  if use_color; then
    printf "$color%b\e[0m\n" "$*"
  else
    printf "%b\n" "$*"
  fi
}

use_color() {
  if [[ -n $COLOR ]]; then
    case $COLOR in
      always) return 0 ;;
      never) return 1 ;;
      *) [[ -t 1 ]] && return 0 || return 1 ;; # anything else is "auto"
    esac
  elif [[ "${NO_COLOR:-}" == "" ]]; then
    return 0
  else
    return 1
  fi
}

red() { print_in_color "\e[31m" "$*"; }
# ... more color functions, as in the original library

This allows the developer to choose what to use, and even to just leave it for the user to decide.

Thoughts?

[DannyBen]

UPDATE: it only works after #610

Ah. Right, I was working with the released version.

[DannyBen]

See #611 with these changes.

[meleu]

I think the enable_auto_colors idea is reasonable and easy to use. I like it! 👍

[pcrockett]

👍 👍

[DannyBen]

Its settled then. Version 1.2.9 will have this option.