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

[Mattijs Korpershoek]

Hi Laurent,

On ven., juin 16, 2023 at 17:55, Laurent Pinchart <laurent.pinchart at 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 at 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 at baylibre.com>
>> 
>> >      if issues:
>
> -- 
> Regards,
>
> Laurent Pinchart