[libcamera-devel,v1,2/4] utils: checkstyle: Support running checkers selectively

18728 diff mbox series

During development of the checkstyle.py script, it can be useful to run
only a subset of the checker. Add the ability to do so with a
'--checkers' command line argument.

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

Hi Laurent,

On mar., juin 13, 2023 at 01:47, Laurent Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:

> During development of the checkstyle.py script, it can be useful to run
> only a subset of the checker. Add the ability to do so with a
> '--checkers' command line argument.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 7da888d8c365..3104acfa2065 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -301,8 +301,10 @@ class CommitChecker(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def checkers(cls):
> +    def checkers(cls, names):
>          for checker in cls.subclasses:
> +            if names and checker.__name__ not in names:
> +                continue
>              yield checker
>  
>  
> @@ -436,8 +438,10 @@ class StyleChecker(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def checkers(cls, filename):
> +    def checkers(cls, filename, names):
>          for checker in cls.subclasses:
> +            if names and checker.__name__ not in names:
> +                continue
>              if checker.supports(filename):
>                  yield checker
>  
> @@ -617,8 +621,10 @@ class Formatter(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def formatters(cls, filename):
> +    def formatters(cls, filename, names):
>          for formatter in cls.subclasses:
> +            if names and formatter.__name__ not in names:
> +                continue
>              if formatter.supports(filename):
>                  yield formatter
>  
> @@ -780,7 +786,7 @@ class StripTrailingSpaceFormatter(Formatter):
>  # Style checking
>  #
>  
> -def check_file(top_level, commit, filename):
> +def check_file(top_level, commit, filename, checkers):
>      # Extract the line numbers touched by the commit.
>      commit_diff = commit.get_diff(top_level, filename)
>  
> @@ -797,7 +803,7 @@ def check_file(top_level, commit, filename):
>      after = commit.get_file(filename)
>  
>      formatted = after
> -    for formatter in Formatter.formatters(filename):
> +    for formatter in Formatter.formatters(filename, checkers):
>          formatted = formatter.format(filename, formatted)
>  
>      after = after.splitlines(True)
> @@ -811,7 +817,7 @@ def check_file(top_level, commit, filename):
>  
>      # Check for code issues not related to formatting.
>      issues = []
> -    for checker in StyleChecker.checkers(filename):
> +    for checker in StyleChecker.checkers(filename, checkers):
>          checker = checker(after)
>          for hunk in commit_diff:
>              issues += checker.check(hunk.side('to').touched)
> @@ -839,7 +845,7 @@ def check_file(top_level, commit, filename):
>      return len(formatted_diff) + len(issues)
>  
>  
> -def check_style(top_level, commit):
> +def check_style(top_level, commit, checkers):
>      separator = '-' * len(commit.title)
>      print(separator)
>      print(commit.title)
> @@ -848,7 +854,7 @@ def check_style(top_level, commit):
>      issues = 0
>  
>      # Apply the commit checkers first.
> -    for checker in CommitChecker.checkers():
> +    for checker in CommitChecker.checkers(checkers):
>          for issue in checker.check(commit, top_level):
>              print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))
>              issues += 1
> @@ -860,7 +866,7 @@ def check_style(top_level, commit):
>      files = [f for f in commit.files() if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]
>  
>      for f in files:
> -        issues += check_file(top_level, commit, f)
> +        issues += check_file(top_level, commit, f, checkers)
>  
>      if issues == 0:
>          print('No issue detected')
> @@ -910,6 +916,8 @@ def main(argv):
>  
>      # Parse command line arguments
>      parser = argparse.ArgumentParser()
> +    parser.add_argument('--checkers', '-c', type=str,
> +                        help='Specify which checkers to run as a comma-separated list. Defaults to all checkers')

Should we also want to add --list-checkers? it might be more convenient
than to have to browse the checkstyle.py code for finding the checker names.

>      parser.add_argument('--staged', '-s', action='store_true',
>                          help='Include the changes in the index. Defaults to False')
>      parser.add_argument('--amend', '-a', action='store_true',
> @@ -918,6 +926,9 @@ def main(argv):
>                          help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
>      args = parser.parse_args(argv[1:])
>  
> +    if args.checkers:
> +        args.checkers = args.checkers.split(',')
> +
>      # Check for required dependencies.
>      for command, mandatory in dependencies.items():
>          found = shutil.which(command)
> @@ -951,7 +962,7 @@ def main(argv):
>  
>      issues = 0
>      for commit in commits:
> -        issues += check_style(top_level, commit)
> +        issues += check_style(top_level, commit, args.checkers)
>          print('')
>  

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

>      if issues:
> -- 
> Regards,
>
> Laurent Pinchart

Hi Mattijs,

On Fri, Jun 16, 2023 at 04:30:16PM +0200, Mattijs Korpershoek wrote:
> On mar., juin 13, 2023 at 01:47, Laurent Pinchart via libcamera-devel wrote:
> 
> > During development of the checkstyle.py script, it can be useful to run
> > only a subset of the checker. Add the ability to do so with a
> > '--checkers' command line argument.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 31 +++++++++++++++++++++----------
> >  1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index 7da888d8c365..3104acfa2065 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -301,8 +301,10 @@ class CommitChecker(metaclass=ClassRegistry):
> >      # Class methods
> >      #
> >      @classmethod
> > -    def checkers(cls):
> > +    def checkers(cls, names):
> >          for checker in cls.subclasses:
> > +            if names and checker.__name__ not in names:
> > +                continue
> >              yield checker
> >  
> >  
> > @@ -436,8 +438,10 @@ class StyleChecker(metaclass=ClassRegistry):
> >      # Class methods
> >      #
> >      @classmethod
> > -    def checkers(cls, filename):
> > +    def checkers(cls, filename, names):
> >          for checker in cls.subclasses:
> > +            if names and checker.__name__ not in names:
> > +                continue
> >              if checker.supports(filename):
> >                  yield checker
> >  
> > @@ -617,8 +621,10 @@ class Formatter(metaclass=ClassRegistry):
> >      # Class methods
> >      #
> >      @classmethod
> > -    def formatters(cls, filename):
> > +    def formatters(cls, filename, names):
> >          for formatter in cls.subclasses:
> > +            if names and formatter.__name__ not in names:
> > +                continue
> >              if formatter.supports(filename):
> >                  yield formatter
> >  
> > @@ -780,7 +786,7 @@ class StripTrailingSpaceFormatter(Formatter):
> >  # Style checking
> >  #
> >  
> > -def check_file(top_level, commit, filename):
> > +def check_file(top_level, commit, filename, checkers):
> >      # Extract the line numbers touched by the commit.
> >      commit_diff = commit.get_diff(top_level, filename)
> >  
> > @@ -797,7 +803,7 @@ def check_file(top_level, commit, filename):
> >      after = commit.get_file(filename)
> >  
> >      formatted = after
> > -    for formatter in Formatter.formatters(filename):
> > +    for formatter in Formatter.formatters(filename, checkers):
> >          formatted = formatter.format(filename, formatted)
> >  
> >      after = after.splitlines(True)
> > @@ -811,7 +817,7 @@ def check_file(top_level, commit, filename):
> >  
> >      # Check for code issues not related to formatting.
> >      issues = []
> > -    for checker in StyleChecker.checkers(filename):
> > +    for checker in StyleChecker.checkers(filename, checkers):
> >          checker = checker(after)
> >          for hunk in commit_diff:
> >              issues += checker.check(hunk.side('to').touched)
> > @@ -839,7 +845,7 @@ def check_file(top_level, commit, filename):
> >      return len(formatted_diff) + len(issues)
> >  
> >  
> > -def check_style(top_level, commit):
> > +def check_style(top_level, commit, checkers):
> >      separator = '-' * len(commit.title)
> >      print(separator)
> >      print(commit.title)
> > @@ -848,7 +854,7 @@ def check_style(top_level, commit):
> >      issues = 0
> >  
> >      # Apply the commit checkers first.
> > -    for checker in CommitChecker.checkers():
> > +    for checker in CommitChecker.checkers(checkers):
> >          for issue in checker.check(commit, top_level):
> >              print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))
> >              issues += 1
> > @@ -860,7 +866,7 @@ def check_style(top_level, commit):
> >      files = [f for f in commit.files() if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]
> >  
> >      for f in files:
> > -        issues += check_file(top_level, commit, f)
> > +        issues += check_file(top_level, commit, f, checkers)
> >  
> >      if issues == 0:
> >          print('No issue detected')
> > @@ -910,6 +916,8 @@ def main(argv):
> >  
> >      # Parse command line arguments
> >      parser = argparse.ArgumentParser()
> > +    parser.add_argument('--checkers', '-c', type=str,
> > +                        help='Specify which checkers to run as a comma-separated list. Defaults to all checkers')
> 
> Should we also want to add --list-checkers? it might be more convenient
> than to have to browse the checkstyle.py code for finding the checker names.

I've thought about it, and I don't mind, but I haven't implemented it as
I've used this new argument to work on checkstyle.py only. I wanted to
run the new checker from this series on all commits in the history, and
running all the checks would have been too slow (and would have
generated too much noise). I'd expect someone who works on adding a new
checker to known the checker name :-)

> >      parser.add_argument('--staged', '-s', action='store_true',
> >                          help='Include the changes in the index. Defaults to False')
> >      parser.add_argument('--amend', '-a', action='store_true',
> > @@ -918,6 +926,9 @@ def main(argv):
> >                          help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
> >      args = parser.parse_args(argv[1:])
> >  
> > +    if args.checkers:
> > +        args.checkers = args.checkers.split(',')
> > +
> >      # Check for required dependencies.
> >      for command, mandatory in dependencies.items():
> >          found = shutil.which(command)
> > @@ -951,7 +962,7 @@ def main(argv):
> >  
> >      issues = 0
> >      for commit in commits:
> > -        issues += check_style(top_level, commit)
> > +        issues += check_style(top_level, commit, args.checkers)
> >          print('')
> >  
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> 
> >      if issues:

Hi Laurent,

On ven., juin 16, 2023 at 17:55, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Fri, Jun 16, 2023 at 04:30:16PM +0200, Mattijs Korpershoek wrote:
>> On mar., juin 13, 2023 at 01:47, Laurent Pinchart via libcamera-devel wrote:
>> 
>> > During development of the checkstyle.py script, it can be useful to run
>> > only a subset of the checker. Add the ability to do so with a
>> > '--checkers' command line argument.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > ---
>> >  utils/checkstyle.py | 31 +++++++++++++++++++++----------
>> >  1 file changed, 21 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
>> > index 7da888d8c365..3104acfa2065 100755
>> > --- a/utils/checkstyle.py
>> > +++ b/utils/checkstyle.py
>> > @@ -301,8 +301,10 @@ class CommitChecker(metaclass=ClassRegistry):
>> >      # Class methods
>> >      #
>> >      @classmethod
>> > -    def checkers(cls):
>> > +    def checkers(cls, names):
>> >          for checker in cls.subclasses:
>> > +            if names and checker.__name__ not in names:
>> > +                continue
>> >              yield checker
>> >  
>> >  
>> > @@ -436,8 +438,10 @@ class StyleChecker(metaclass=ClassRegistry):
>> >      # Class methods
>> >      #
>> >      @classmethod
>> > -    def checkers(cls, filename):
>> > +    def checkers(cls, filename, names):
>> >          for checker in cls.subclasses:
>> > +            if names and checker.__name__ not in names:
>> > +                continue
>> >              if checker.supports(filename):
>> >                  yield checker
>> >  
>> > @@ -617,8 +621,10 @@ class Formatter(metaclass=ClassRegistry):
>> >      # Class methods
>> >      #
>> >      @classmethod
>> > -    def formatters(cls, filename):
>> > +    def formatters(cls, filename, names):
>> >          for formatter in cls.subclasses:
>> > +            if names and formatter.__name__ not in names:
>> > +                continue
>> >              if formatter.supports(filename):
>> >                  yield formatter
>> >  
>> > @@ -780,7 +786,7 @@ class StripTrailingSpaceFormatter(Formatter):
>> >  # Style checking
>> >  #
>> >  
>> > -def check_file(top_level, commit, filename):
>> > +def check_file(top_level, commit, filename, checkers):
>> >      # Extract the line numbers touched by the commit.
>> >      commit_diff = commit.get_diff(top_level, filename)
>> >  
>> > @@ -797,7 +803,7 @@ def check_file(top_level, commit, filename):
>> >      after = commit.get_file(filename)
>> >  
>> >      formatted = after
>> > -    for formatter in Formatter.formatters(filename):
>> > +    for formatter in Formatter.formatters(filename, checkers):
>> >          formatted = formatter.format(filename, formatted)
>> >  
>> >      after = after.splitlines(True)
>> > @@ -811,7 +817,7 @@ def check_file(top_level, commit, filename):
>> >  
>> >      # Check for code issues not related to formatting.
>> >      issues = []
>> > -    for checker in StyleChecker.checkers(filename):
>> > +    for checker in StyleChecker.checkers(filename, checkers):
>> >          checker = checker(after)
>> >          for hunk in commit_diff:
>> >              issues += checker.check(hunk.side('to').touched)
>> > @@ -839,7 +845,7 @@ def check_file(top_level, commit, filename):
>> >      return len(formatted_diff) + len(issues)
>> >  
>> >  
>> > -def check_style(top_level, commit):
>> > +def check_style(top_level, commit, checkers):
>> >      separator = '-' * len(commit.title)
>> >      print(separator)
>> >      print(commit.title)
>> > @@ -848,7 +854,7 @@ def check_style(top_level, commit):
>> >      issues = 0
>> >  
>> >      # Apply the commit checkers first.
>> > -    for checker in CommitChecker.checkers():
>> > +    for checker in CommitChecker.checkers(checkers):
>> >          for issue in checker.check(commit, top_level):
>> >              print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))
>> >              issues += 1
>> > @@ -860,7 +866,7 @@ def check_style(top_level, commit):
>> >      files = [f for f in commit.files() if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]
>> >  
>> >      for f in files:
>> > -        issues += check_file(top_level, commit, f)
>> > +        issues += check_file(top_level, commit, f, checkers)
>> >  
>> >      if issues == 0:
>> >          print('No issue detected')
>> > @@ -910,6 +916,8 @@ def main(argv):
>> >  
>> >      # Parse command line arguments
>> >      parser = argparse.ArgumentParser()
>> > +    parser.add_argument('--checkers', '-c', type=str,
>> > +                        help='Specify which checkers to run as a comma-separated list. Defaults to all checkers')
>> 
>> Should we also want to add --list-checkers? it might be more convenient
>> than to have to browse the checkstyle.py code for finding the checker names.
>
> I've thought about it, and I don't mind, but I haven't implemented it as
> I've used this new argument to work on checkstyle.py only. I wanted to
> run the new checker from this series on all commits in the history, and
> running all the checks would have been too slow (and would have
> generated too much noise). I'd expect someone who works on adding a new
> checker to known the checker name :-)

Indeed. Adding --list-checkers is definitely out of scope for this
patch series.

Thanks for the explanation !

>
>> >      parser.add_argument('--staged', '-s', action='store_true',
>> >                          help='Include the changes in the index. Defaults to False')
>> >      parser.add_argument('--amend', '-a', action='store_true',
>> > @@ -918,6 +926,9 @@ def main(argv):
>> >                          help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
>> >      args = parser.parse_args(argv[1:])
>> >  
>> > +    if args.checkers:
>> > +        args.checkers = args.checkers.split(',')
>> > +
>> >      # Check for required dependencies.
>> >      for command, mandatory in dependencies.items():
>> >          found = shutil.which(command)
>> > @@ -951,7 +962,7 @@ def main(argv):
>> >  
>> >      issues = 0
>> >      for commit in commits:
>> > -        issues += check_style(top_level, commit)
>> > +        issues += check_style(top_level, commit, args.checkers)
>> >          print('')
>> >  
>> 
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> 
>> >      if issues:
>
> -- 
> Regards,
>
> Laurent Pinchart

Quoting Laurent Pinchart via libcamera-devel (2023-06-12 23:47:49)
> During development of the checkstyle.py script, it can be useful to run
> only a subset of the checker. Add the ability to do so with a
> '--checkers' command line argument.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 7da888d8c365..3104acfa2065 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -301,8 +301,10 @@ class CommitChecker(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def checkers(cls):
> +    def checkers(cls, names):
>          for checker in cls.subclasses:
> +            if names and checker.__name__ not in names:

Seems it was hard for me to see that 'names' can be null if
args.checkers is not set, but I found it in the end.

As suggested by Matti, a way to list the names might be useful - but as
it's to allow running a single checker while the developer is working on
/that/ checker - I can't see a reason to limit/filter otherwise right
now - I would want all capable checkers to run in the normal case so:

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

> +                continue
>              yield checker
>  
>  
> @@ -436,8 +438,10 @@ class StyleChecker(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def checkers(cls, filename):
> +    def checkers(cls, filename, names):
>          for checker in cls.subclasses:
> +            if names and checker.__name__ not in names:
> +                continue
>              if checker.supports(filename):
>                  yield checker
>  
> @@ -617,8 +621,10 @@ class Formatter(metaclass=ClassRegistry):
>      # Class methods
>      #
>      @classmethod
> -    def formatters(cls, filename):
> +    def formatters(cls, filename, names):
>          for formatter in cls.subclasses:
> +            if names and formatter.__name__ not in names:
> +                continue
>              if formatter.supports(filename):
>                  yield formatter
>  
> @@ -780,7 +786,7 @@ class StripTrailingSpaceFormatter(Formatter):
>  # Style checking
>  #
>  
> -def check_file(top_level, commit, filename):
> +def check_file(top_level, commit, filename, checkers):
>      # Extract the line numbers touched by the commit.
>      commit_diff = commit.get_diff(top_level, filename)
>  
> @@ -797,7 +803,7 @@ def check_file(top_level, commit, filename):
>      after = commit.get_file(filename)
>  
>      formatted = after
> -    for formatter in Formatter.formatters(filename):
> +    for formatter in Formatter.formatters(filename, checkers):
>          formatted = formatter.format(filename, formatted)
>  
>      after = after.splitlines(True)
> @@ -811,7 +817,7 @@ def check_file(top_level, commit, filename):
>  
>      # Check for code issues not related to formatting.
>      issues = []
> -    for checker in StyleChecker.checkers(filename):
> +    for checker in StyleChecker.checkers(filename, checkers):
>          checker = checker(after)
>          for hunk in commit_diff:
>              issues += checker.check(hunk.side('to').touched)
> @@ -839,7 +845,7 @@ def check_file(top_level, commit, filename):
>      return len(formatted_diff) + len(issues)
>  
>  
> -def check_style(top_level, commit):
> +def check_style(top_level, commit, checkers):
>      separator = '-' * len(commit.title)
>      print(separator)
>      print(commit.title)
> @@ -848,7 +854,7 @@ def check_style(top_level, commit):
>      issues = 0
>  
>      # Apply the commit checkers first.
> -    for checker in CommitChecker.checkers():
> +    for checker in CommitChecker.checkers(checkers):
>          for issue in checker.check(commit, top_level):
>              print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))
>              issues += 1
> @@ -860,7 +866,7 @@ def check_style(top_level, commit):
>      files = [f for f in commit.files() if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]
>  
>      for f in files:
> -        issues += check_file(top_level, commit, f)
> +        issues += check_file(top_level, commit, f, checkers)
>  
>      if issues == 0:
>          print('No issue detected')
> @@ -910,6 +916,8 @@ def main(argv):
>  
>      # Parse command line arguments
>      parser = argparse.ArgumentParser()
> +    parser.add_argument('--checkers', '-c', type=str,
> +                        help='Specify which checkers to run as a comma-separated list. Defaults to all checkers')
>      parser.add_argument('--staged', '-s', action='store_true',
>                          help='Include the changes in the index. Defaults to False')
>      parser.add_argument('--amend', '-a', action='store_true',
> @@ -918,6 +926,9 @@ def main(argv):
>                          help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
>      args = parser.parse_args(argv[1:])
>  
> +    if args.checkers:
> +        args.checkers = args.checkers.split(',')
> +
>      # Check for required dependencies.
>      for command, mandatory in dependencies.items():
>          found = shutil.which(command)
> @@ -951,7 +962,7 @@ def main(argv):
>  
>      issues = 0
>      for commit in commits:
> -        issues += check_style(top_level, commit)
> +        issues += check_style(top_level, commit, args.checkers)
>          print('')
>  
>      if issues:
> -- 
> Regards,
> 
> Laurent Pinchart
>