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. ;)
> So there seem to be two things here: double return instead of
> conditional expression, and calling the isatty method for every relevant
> log line instead of once at the beginning.
>
> I deliberately avoided the latter and I think I still prefer the
> conditional expression to multiple return statements, too, but either
> way should keep the escape sequences out of the log files and I don't
> feel strongly about it.
Oh, you're right. I missed the fact about repeated isatty call, which is
certainly worth keeping. One other thing I just noticed, you're dropping
the other colors' definitions. Maybe worth keeping them? 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.
Cheers, Phil
