| Message ID | 20220530072228.15916-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, On Mon, May 30, 2022 at 10:22:28AM +0300, Tomi Valkeinen via libcamera-devel wrote: > If issue.line is None, the the terminal color is never reset back to > normal. This causes the yellow color to bleed. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > utils/checkstyle.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py > index 835f2a9f..fa0513f2 100755 > --- a/utils/checkstyle.py > +++ b/utils/checkstyle.py > @@ -743,9 +743,9 @@ def check_file(top_level, commit, filename): > if len(issues): > issues = sorted(issues, key=lambda i: i.line_number) > for issue in issues: > - print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg)) > + print('%s#%u: %s%s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg, Colours.reset())) > if issue.line is not None: > - print('+%s%s' % (issue.line.rstrip(), Colours.reset())) > + print('%s+%s%s' % (Colours.fg(Colours.Yellow), issue.line.rstrip(), Colours.reset())) > > return len(formatted_diff) + len(issues) > > -- > 2.34.1 >
Hi Tomi, Thank you for the patch. On Mon, May 30, 2022 at 10:22:28AM +0300, Tomi Valkeinen wrote: > If issue.line is None, the the terminal color is never reset back to > normal. This causes the yellow color to bleed. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > utils/checkstyle.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py > index 835f2a9f..fa0513f2 100755 > --- a/utils/checkstyle.py > +++ b/utils/checkstyle.py > @@ -743,9 +743,9 @@ def check_file(top_level, commit, filename): > if len(issues): > issues = sorted(issues, key=lambda i: i.line_number) > for issue in issues: > - print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg)) > + print('%s#%u: %s%s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg, Colours.reset())) This could be wrapped. > if issue.line is not None: > - print('+%s%s' % (issue.line.rstrip(), Colours.reset())) > + print('%s+%s%s' % (Colours.fg(Colours.Yellow), issue.line.rstrip(), Colours.reset())) I think I would have added sys.stdout.write(Colours.reset()) here (and dropped it from the previous line) to avoid outputting it twice, but that makes no big difference. Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > return len(formatted_diff) + len(issues) >
On 30/05/2022 11:22, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, May 30, 2022 at 10:22:28AM +0300, Tomi Valkeinen wrote: >> If issue.line is None, the the terminal color is never reset back to >> normal. This causes the yellow color to bleed. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> utils/checkstyle.py | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/utils/checkstyle.py b/utils/checkstyle.py >> index 835f2a9f..fa0513f2 100755 >> --- a/utils/checkstyle.py >> +++ b/utils/checkstyle.py >> @@ -743,9 +743,9 @@ def check_file(top_level, commit, filename): >> if len(issues): >> issues = sorted(issues, key=lambda i: i.line_number) >> for issue in issues: >> - print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg)) >> + print('%s#%u: %s%s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg, Colours.reset())) > > This could be wrapped. > >> if issue.line is not None: >> - print('+%s%s' % (issue.line.rstrip(), Colours.reset())) >> + print('%s+%s%s' % (Colours.fg(Colours.Yellow), issue.line.rstrip(), Colours.reset())) > > I think I would have added > > sys.stdout.write(Colours.reset()) > > here (and dropped it from the previous line) to avoid outputting it > twice, but that makes no big difference. Either way, There are many ways to do this, of course. If we want to optimize, we'll print Yellow only once, then the text lines, and a reset at the end. But that would be a rather pointless optimization. I thought it's most obvious for the reader and easiest to maintain if we print a color and a reset in the same print() call. In fact, I think it would be better to have a function, colorize(color, str), which returns the str with color and reset. This is what e.g. xtermcolor module does. Tomi
Hi Tomi, On Fri, Jun 03, 2022 at 09:09:24AM +0300, Tomi Valkeinen wrote: > On 30/05/2022 11:22, Laurent Pinchart wrote: > > On Mon, May 30, 2022 at 10:22:28AM +0300, Tomi Valkeinen wrote: > >> If issue.line is None, the the terminal color is never reset back to > >> normal. This causes the yellow color to bleed. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> utils/checkstyle.py | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/utils/checkstyle.py b/utils/checkstyle.py > >> index 835f2a9f..fa0513f2 100755 > >> --- a/utils/checkstyle.py > >> +++ b/utils/checkstyle.py > >> @@ -743,9 +743,9 @@ def check_file(top_level, commit, filename): > >> if len(issues): > >> issues = sorted(issues, key=lambda i: i.line_number) > >> for issue in issues: > >> - print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg)) > >> + print('%s#%u: %s%s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg, Colours.reset())) > > > > This could be wrapped. > > > >> if issue.line is not None: > >> - print('+%s%s' % (issue.line.rstrip(), Colours.reset())) > >> + print('%s+%s%s' % (Colours.fg(Colours.Yellow), issue.line.rstrip(), Colours.reset())) > > > > I think I would have added > > > > sys.stdout.write(Colours.reset()) > > > > here (and dropped it from the previous line) to avoid outputting it > > twice, but that makes no big difference. Either way, > > There are many ways to do this, of course. If we want to optimize, we'll > print Yellow only once, then the text lines, and a reset at the end. But > that would be a rather pointless optimization. > > I thought it's most obvious for the reader and easiest to maintain if we > print a color and a reset in the same print() call. > > In fact, I think it would be better to have a function, colorize(color, > str), which returns the str with color and reset. This is what e.g. > xtermcolor module does. That's a good point. There's clearly no need to optimize this for performance, so if someone wants to add a colorize() function, I'll be happy to merge it. In the meantime I'll merge your patch with the line wrap I proposed.
diff --git a/utils/checkstyle.py b/utils/checkstyle.py index 835f2a9f..fa0513f2 100755 --- a/utils/checkstyle.py +++ b/utils/checkstyle.py @@ -743,9 +743,9 @@ def check_file(top_level, commit, filename): if len(issues): issues = sorted(issues, key=lambda i: i.line_number) for issue in issues: - print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg)) + print('%s#%u: %s%s' % (Colours.fg(Colours.Yellow), issue.line_number, issue.msg, Colours.reset())) if issue.line is not None: - print('+%s%s' % (issue.line.rstrip(), Colours.reset())) + print('%s+%s%s' % (Colours.fg(Colours.Yellow), issue.line.rstrip(), Colours.reset())) return len(formatted_diff) + len(issues)
If issue.line is None, the the terminal color is never reset back to normal. This causes the yellow color to bleed. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- utils/checkstyle.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)