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

[Kieran Bingham]

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

> Signed-off-by: Laurent Pinchart <laurent.pinchart at 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):
> 

-- 
Regards
--
Kieran