Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

["Štěpán Němec" <snemec@redhat.com>]

On Fri, 03 Sep 2021 17:34:20 +0200
Phil Sutter wrote:

> On Fri, Sep 03, 2021 at 04:44:41PM +0200, Štěpán Němec wrote:
>> On Fri, 03 Sep 2021 14:52:50 +0200
>> Phil Sutter wrote:
>> 
>> > With one minor nit:
>> >
>> >> diff --git a/iptables-test.py b/iptables-test.py
>> >> index 90e07feed365..e8fc0c75a43e 100755
>> >> --- a/iptables-test.py
>> >> +++ b/iptables-test.py
>> >> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
>> >>  LOGFILE="/tmp/iptables-test.log"
>> >>  log_file = None
>> >>  
>> >> +STDOUT_IS_TTY = sys.stdout.isatty()
>> >>  
>> >> -class Colors:
>> >> -    HEADER = '\033[95m'
>> >> -    BLUE = '\033[94m'
>> >> -    GREEN = '\033[92m'
>> >> -    YELLOW = '\033[93m'
>> >> -    RED = '\033[91m'
>> >> -    ENDC = '\033[0m'
>> >> +def maybe_colored(color, text):
>> >> +    terminal_sequences = {
>> >> +        'green': '\033[92m',
>> >> +        'red': '\033[91m',
>> >> +    }
>> >> +
>> >> +    return (
>> >> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
>> >> +    )
>> >
>> > I would "simplify" this into:
>> >
>> > | if not sys.stdout.isatty():
>> > | 	return text
>> > | return terminal_sequences[color] + text + '\033[0m'
>> 
>> ...which could be further simplified by dropping 'not' and swapping the
>> two branches.
>
> My change was mostly about reducing the long line, i.e. cosmetics. ;)

Ah, I see. I agree it's not the prettiest thing, but it's still in 80
columns (something that can't be said about a few other lines in the
script).

> One other thing I just noticed, you're dropping the other colors'
> definitions. Maybe worth keeping them?

Is it? I couldn't find anything but red and green ever being used since
2012 when the file was added.

> Also I'm not too happy about the Python exception if an unknown color
> name is passed to maybe_colored(). OTOH though it's just a test script
> and such bug is easily identified.

Yes, it's a helper function in a test script, not some kind of public
API. I don't see how maybe_colored('magenta', ...) is different in that
respect or more likely than print(Colors.MAGENTA + ...) previously.

-- 
Štěpán