[libcamera-devel] [PATCH v1 2/4] utils: checkstyle: Support running checkers selectively
Mattijs Korpershoek
mkorpershoek at baylibre.com
Mon Jun 19 09:17:51 CEST 2023
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
More information about the libcamera-devel
mailing list