Hi,
On Mon, Sep 06, 2021 at 11:04:38AM +0200, Štěpán Němec wrote:
> 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.
OK, fine. I'm convinced. :)
Applied, thanks!
