[libcamera-devel,2/2] utils: checkstyle: Catch LOG() usage without an explicit category

559 diff mbox series

Add support for checkers not related to code formatting to the
checkstyle.py script, and create a first checker that catches usage of
the LOG() macro without an explicit category.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/checkstyle.py | 55 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 8 deletions(-)

Hi Laurent,

On 12/02/2019 22:25, Laurent Pinchart wrote:
> Add support for checkers not related to code formatting to the
> checkstyle.py script, and create a first checker that catches usage of
> the LOG() macro without an explicit category.

Ah, now I see why you said it was easy to add extra checkers.

It's easy when you know the architecture of the script at least :)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 55 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index f3005d1fbb52..9abd2687b1f9 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -146,7 +146,7 @@ class DiffHunk(object):
>              s += Colours.reset()
>              s += '\n'
>  
> -        return s
> +        return s[:-1]

I think this chops off the last character in s. Why are you doing that ?
(especially as without seeing the rest of the context, you've only
'just' added a '\n'

Or is that it - to remove the last \n ?


Aha ok it clearly is to remove the \n.

>      def append(self, line):
>          if line[0] == ' ':
> @@ -229,6 +229,29 @@ available_formatters = {
>  # Style checking
>  #
>  
> +class LogCategoryChecker(object):
> +    log_regex = re.compile('\\bLOG\((Debug|Info|Warning|Error|Fatal)\)')
> +
> +    def __init__(self, content):
> +        self.__content = content
> +
> +    def check(self, line_numbers):
> +        issues = []
> +        for line_number in line_numbers:
> +            line = self.__content[line_number-1]
> +            if not LogCategoryChecker.log_regex.search(line):
> +                continue
> +
> +            issues.append([line_number, line, 'LOG() should use categories'])
> +
> +        return issues
> +
> +
> +available_checkers = {
> +    'log_category': LogCategoryChecker,
> +}
> +
> +
>  def check_file(top_level, commit, filename, formatters):
>      # Extract the line numbers touched by the commit.
>      diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> @@ -260,20 +283,36 @@ def check_file(top_level, commit, filename, formatters):
>      formatted = formatted.splitlines(True)
>      diff = difflib.unified_diff(after, formatted)
>  
> -    # Split the diff in hunks, recording line number ranges for each hunk.
> +    # Split the diff in hunks, recording line number ranges for each hunk, and
> +    # filter out hunks that are not touched by the commit.
>      formatted_diff = parse_diff(diff)
> -
> -    # Filter out hunks that are not touched by the commit.
>      formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)]
> -    if len(formatted_diff) == 0:
> +
> +    # Check for code issues not related to formatting.
> +    issues = []
> +    for checker in available_checkers:
> +        checker = available_checkers[checker](after)
> +        for hunk in commit_diff:
> +            issues += checker.check(hunk.side('to').touched)
> +
> +    # Print the detected issues.
> +    if len(issues) == 0 and len(formatted_diff) == 0:
>          return 0
>  
>      print('%s---' % Colours.fg(Colours.Red), filename)
>      print('%s+++' % Colours.fg(Colours.Green), filename)

Interestingly, these both print red and green respectively...

> -    for hunk in formatted_diff:
> -        print(hunk)
>  
> -    return len(formatted_diff)
> +    if len(formatted_diff):
> +        for hunk in formatted_diff:
> +            print(hunk)
> +
> +    if len(issues):
> +        issues.sort()
> +        for issue in issues:
> +            print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue[0], issue[2]))

But this looks distinctly brown or is it orange ...
I wonder if that's due to my shell perhaps.

> +            print('+%s%s' % (issue[1].rstrip(), Colours.reset()))
> +
> +    return len(formatted_diff) + len(issues)
>  
>  
>  def check_style(top_level, commit, formatters):
>

Hi Laurent,

Thanks for your patch.

On 2019-02-13 00:25:57 +0200, Laurent Pinchart wrote:
> Add support for checkers not related to code formatting to the
> checkstyle.py script, and create a first checker that catches usage of
> the LOG() macro without an explicit category.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

With Kierans explanation on the change in DiffHunk

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

I like how we can add checks like this, nice work.

> ---
>  utils/checkstyle.py | 55 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index f3005d1fbb52..9abd2687b1f9 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -146,7 +146,7 @@ class DiffHunk(object):
>              s += Colours.reset()
>              s += '\n'
>  
> -        return s
> +        return s[:-1]
>  
>      def append(self, line):
>          if line[0] == ' ':
> @@ -229,6 +229,29 @@ available_formatters = {
>  # Style checking
>  #
>  
> +class LogCategoryChecker(object):
> +    log_regex = re.compile('\\bLOG\((Debug|Info|Warning|Error|Fatal)\)')
> +
> +    def __init__(self, content):
> +        self.__content = content
> +
> +    def check(self, line_numbers):
> +        issues = []
> +        for line_number in line_numbers:
> +            line = self.__content[line_number-1]
> +            if not LogCategoryChecker.log_regex.search(line):
> +                continue
> +
> +            issues.append([line_number, line, 'LOG() should use categories'])
> +
> +        return issues
> +
> +
> +available_checkers = {
> +    'log_category': LogCategoryChecker,
> +}
> +
> +
>  def check_file(top_level, commit, filename, formatters):
>      # Extract the line numbers touched by the commit.
>      diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> @@ -260,20 +283,36 @@ def check_file(top_level, commit, filename, formatters):
>      formatted = formatted.splitlines(True)
>      diff = difflib.unified_diff(after, formatted)
>  
> -    # Split the diff in hunks, recording line number ranges for each hunk.
> +    # Split the diff in hunks, recording line number ranges for each hunk, and
> +    # filter out hunks that are not touched by the commit.
>      formatted_diff = parse_diff(diff)
> -
> -    # Filter out hunks that are not touched by the commit.
>      formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)]
> -    if len(formatted_diff) == 0:
> +
> +    # Check for code issues not related to formatting.
> +    issues = []
> +    for checker in available_checkers:
> +        checker = available_checkers[checker](after)
> +        for hunk in commit_diff:
> +            issues += checker.check(hunk.side('to').touched)
> +
> +    # Print the detected issues.
> +    if len(issues) == 0 and len(formatted_diff) == 0:
>          return 0
>  
>      print('%s---' % Colours.fg(Colours.Red), filename)
>      print('%s+++' % Colours.fg(Colours.Green), filename)
> -    for hunk in formatted_diff:
> -        print(hunk)
>  
> -    return len(formatted_diff)
> +    if len(formatted_diff):
> +        for hunk in formatted_diff:
> +            print(hunk)
> +
> +    if len(issues):
> +        issues.sort()
> +        for issue in issues:
> +            print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue[0], issue[2]))
> +            print('+%s%s' % (issue[1].rstrip(), Colours.reset()))
> +
> +    return len(formatted_diff) + len(issues)
>  
>  
>  def check_style(top_level, commit, formatters):
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Wed, Feb 13, 2019 at 08:17:32AM +0000, Kieran Bingham wrote:
> On 12/02/2019 22:25, Laurent Pinchart wrote:
> > Add support for checkers not related to code formatting to the
> > checkstyle.py script, and create a first checker that catches usage of
> > the LOG() macro without an explicit category.
> 
> Ah, now I see why you said it was easy to add extra checkers.
> 
> It's easy when you know the architecture of the script at least :)

Feel free to get yourself familiar with it too ;-)

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 55 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 47 insertions(+), 8 deletions(-)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index f3005d1fbb52..9abd2687b1f9 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -146,7 +146,7 @@ class DiffHunk(object):
> >              s += Colours.reset()
> >              s += '\n'
> >  
> > -        return s
> > +        return s[:-1]
> 
> I think this chops off the last character in s. Why are you doing that ?
> (especially as without seeing the rest of the context, you've only
> 'just' added a '\n'
> 
> Or is that it - to remove the last \n ?
> 
> 
> Aha ok it clearly is to remove the \n.

Yes, as the string is printed with the print() function, an extra \n is
added.

> >      def append(self, line):
> >          if line[0] == ' ':
> > @@ -229,6 +229,29 @@ available_formatters = {
> >  # Style checking
> >  #
> >  
> > +class LogCategoryChecker(object):
> > +    log_regex = re.compile('\\bLOG\((Debug|Info|Warning|Error|Fatal)\)')
> > +
> > +    def __init__(self, content):
> > +        self.__content = content
> > +
> > +    def check(self, line_numbers):
> > +        issues = []
> > +        for line_number in line_numbers:
> > +            line = self.__content[line_number-1]
> > +            if not LogCategoryChecker.log_regex.search(line):
> > +                continue
> > +
> > +            issues.append([line_number, line, 'LOG() should use categories'])
> > +
> > +        return issues
> > +
> > +
> > +available_checkers = {
> > +    'log_category': LogCategoryChecker,
> > +}
> > +
> > +
> >  def check_file(top_level, commit, filename, formatters):
> >      # Extract the line numbers touched by the commit.
> >      diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> > @@ -260,20 +283,36 @@ def check_file(top_level, commit, filename, formatters):
> >      formatted = formatted.splitlines(True)
> >      diff = difflib.unified_diff(after, formatted)
> >  
> > -    # Split the diff in hunks, recording line number ranges for each hunk.
> > +    # Split the diff in hunks, recording line number ranges for each hunk, and
> > +    # filter out hunks that are not touched by the commit.
> >      formatted_diff = parse_diff(diff)
> > -
> > -    # Filter out hunks that are not touched by the commit.
> >      formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)]
> > -    if len(formatted_diff) == 0:
> > +
> > +    # Check for code issues not related to formatting.
> > +    issues = []
> > +    for checker in available_checkers:
> > +        checker = available_checkers[checker](after)
> > +        for hunk in commit_diff:
> > +            issues += checker.check(hunk.side('to').touched)
> > +
> > +    # Print the detected issues.
> > +    if len(issues) == 0 and len(formatted_diff) == 0:
> >          return 0
> >  
> >      print('%s---' % Colours.fg(Colours.Red), filename)
> >      print('%s+++' % Colours.fg(Colours.Green), filename)
> 
> Interestingly, these both print red and green respectively...
> 
> > -    for hunk in formatted_diff:
> > -        print(hunk)
> >  
> > -    return len(formatted_diff)
> > +    if len(formatted_diff):
> > +        for hunk in formatted_diff:
> > +            print(hunk)
> > +
> > +    if len(issues):
> > +        issues.sort()
> > +        for issue in issues:
> > +            print('%s#%u: %s' % (Colours.fg(Colours.Yellow), issue[0], issue[2]))
> 
> But this looks distinctly brown or is it orange ...
> I wonder if that's due to my shell perhaps.

It prints orange-brown here, probably the same colour as on your screen.
The colour being displayed depends on your terminal's theme.

> > +            print('+%s%s' % (issue[1].rstrip(), Colours.reset()))
> > +
> > +    return len(formatted_diff) + len(issues)
> >  
> >  
> >  def check_style(top_level, commit, formatters):