[libcamera-devel] [PATCH] utils: checkstyle.py: Fix color bleed

[Laurent Pinchart]

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 at 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.

-- 
Regards,

Laurent Pinchart