* [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
@ 2021-09-02 11:33 Štěpán Němec
2021-09-03 12:52 ` Phil Sutter
0 siblings, 1 reply; 9+ messages in thread
From: Štěpán Němec @ 2021-09-02 11:33 UTC (permalink / raw)
To: netfilter-devel, Pablo Neira Ayuso
When the output doesn't go to a terminal (typical case: log files),
the escape sequences are just noise.
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
iptables-test.py | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
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
+ )
def print_error(reason, filename=None, lineno=None):
'''
Prints an error with nice colors, indicating file and line number.
'''
- print(filename + ": " + Colors.RED + "ERROR" +
- Colors.ENDC + ": line %d (%s)" % (lineno, reason))
+ print(filename + ": " + maybe_colored('red', "ERROR") +
+ ": line %d (%s)" % (lineno, reason))
def delete_rule(iptables, rule, filename, lineno):
@@ -282,7 +285,7 @@ def run_test_file(filename, netns):
if netns:
execute_cmd("ip netns del ____iptables-container-test", filename, 0)
if total_test_passed:
- print(filename + ": " + Colors.GREEN + "OK" + Colors.ENDC)
+ print(filename + ": " + maybe_colored('green', "OK"))
f.close()
return tests, passed
base-commit: e438b9766fcc86d9847312ff05f1d1dac61acf1f
--
2.33.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-02 11:33 [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty Štěpán Němec
@ 2021-09-03 12:52 ` Phil Sutter
2021-09-03 14:44 ` Štěpán Němec
0 siblings, 1 reply; 9+ messages in thread
From: Phil Sutter @ 2021-09-03 12:52 UTC (permalink / raw)
To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso
Hi,
On Thu, Sep 02, 2021 at 01:33:07PM +0200, Štěpán Němec wrote:
> When the output doesn't go to a terminal (typical case: log files),
> the escape sequences are just noise.
>
> Signed-off-by: Štěpán Němec <snemec@redhat.com>
Acked-by: Phil Sutter <phil@nwl.cc>
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'
But what's beautiful in C might be ugly in Python and I'm blind to that
aspect. ;)
Cheers, Phil
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-03 12:52 ` Phil Sutter
@ 2021-09-03 14:44 ` Štěpán Němec
2021-09-03 15:34 ` Phil Sutter
0 siblings, 1 reply; 9+ messages in thread
From: Štěpán Němec @ 2021-09-03 14:44 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso
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.
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.
Thanks,
Štěpán
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-03 14:44 ` Štěpán Němec
@ 2021-09-03 15:34 ` Phil Sutter
2021-09-06 9:04 ` Štěpán Němec
0 siblings, 1 reply; 9+ messages in thread
From: Phil Sutter @ 2021-09-03 15:34 UTC (permalink / raw)
To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-03 15:34 ` Phil Sutter
@ 2021-09-06 9:04 ` Štěpán Němec
2021-09-13 15:05 ` Phil Sutter
0 siblings, 1 reply; 9+ messages in thread
From: Štěpán Němec @ 2021-09-06 9:04 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-06 9:04 ` Štěpán Němec
@ 2021-09-13 15:05 ` Phil Sutter
2021-09-14 9:03 ` Štěpán Němec
0 siblings, 1 reply; 9+ messages in thread
From: Phil Sutter @ 2021-09-13 15:05 UTC (permalink / raw)
To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-13 15:05 ` Phil Sutter
@ 2021-09-14 9:03 ` Štěpán Němec
2021-09-14 11:25 ` Phil Sutter
0 siblings, 1 reply; 9+ messages in thread
From: Štěpán Němec @ 2021-09-14 9:03 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso
On Mon, 13 Sep 2021 17:05:33 +0200
Phil Sutter wrote:
> Applied, thanks!
Thank you.
I see that you've pushed your series including the change to print error
messages to stdout [1] in the meantime.
I don't have a strong opinion on whether output of a script whose
(only?) purpose is to print diagnostic messages should go to stdout or
stderr, but I do think that having the "ERROR"s go to stderr and "OK"s
go to stdout is more confusing than useful: was that really intentional?
As a side effect of that change, my patch will act funny depending on
which output stream is being redirected, too.
(I'm sorry I haven't pointed this out earlier; I just skimmed your
patches and didn't notice this until double checking the conflict/merge
with my patch now.)
--
Štěpán
[1]
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210906163038.15381-4-phil@nwl.cc/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-14 9:03 ` Štěpán Němec
@ 2021-09-14 11:25 ` Phil Sutter
2021-09-14 11:49 ` Štěpán Němec
0 siblings, 1 reply; 9+ messages in thread
From: Phil Sutter @ 2021-09-14 11:25 UTC (permalink / raw)
To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso
Hey,
On Tue, Sep 14, 2021 at 11:03:42AM +0200, Štěpán Němec wrote:
> On Mon, 13 Sep 2021 17:05:33 +0200
> Phil Sutter wrote:
>
> > Applied, thanks!
>
> Thank you.
>
> I see that you've pushed your series including the change to print error
> messages to stdout [1] in the meantime.
>
> I don't have a strong opinion on whether output of a script whose
> (only?) purpose is to print diagnostic messages should go to stdout or
> stderr, but I do think that having the "ERROR"s go to stderr and "OK"s
> go to stdout is more confusing than useful: was that really intentional?
>
> As a side effect of that change, my patch will act funny depending on
> which output stream is being redirected, too.
Argh, you're right and I missed that.
Printing errors to stderr is useful to compare failing tests against an
expected set of failures - it is simply a task of comparing output on
stderr with a recorded one.
To not overcomplicate things, maybe the easiest fix would be to print
colors only if both stdout and stderr are a tty. What do you think?
Cheers, Phil
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty
2021-09-14 11:25 ` Phil Sutter
@ 2021-09-14 11:49 ` Štěpán Němec
0 siblings, 0 replies; 9+ messages in thread
From: Štěpán Němec @ 2021-09-14 11:49 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso
On Tue, 14 Sep 2021 13:25:16 +0200
Phil Sutter wrote:
> Printing errors to stderr is useful to compare failing tests against an
> expected set of failures - it is simply a task of comparing output on
> stderr with a recorded one.
I see. I'm still not sure the expected convenience factor (avoiding some
grep-like post processing? but couldn't you compare the combined output
just the same?) outweighs the weirdness / least surprise factor (having
two variations of the same diagnostic message split on two separate
output streams).
> To not overcomplicate things, maybe the easiest fix would be to print
> colors only if both stdout and stderr are a tty. What do you think?
Yes, if the split has to stay, I don't have a better suggestion.
Thanks,
Štěpán
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-14 11:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 11:33 [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty Štěpán Němec
2021-09-03 12:52 ` Phil Sutter
2021-09-03 14:44 ` Štěpán Němec
2021-09-03 15:34 ` Phil Sutter
2021-09-06 9:04 ` Štěpán Němec
2021-09-13 15:05 ` Phil Sutter
2021-09-14 9:03 ` Štěpán Němec
2021-09-14 11:25 ` Phil Sutter
2021-09-14 11:49 ` Štěpán Němec
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.