[PATCH 3/3] utils: checkstyle: Add a python formatter

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Fri, Aug 30, 2024 at 06:06:28PM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Aug 30, 2024 at 02:53:00PM +0200, Stefan Klug wrote:
> > Reporting style issues on python files is great, automatically fixing
> > them is even better. Add a call to autopep8 for python files.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index 5901f1a71562..ed15145b64a4 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -954,6 +954,21 @@ class StripTrailingSpaceFormatter(Formatter):
> >          return ''.join(lines)
> >  
> >  
> > +class Pep8Formatter(Formatter):
> > +    patterns = ('*.py',)
> > +
> > +    @classmethod
> > +    def format(cls, filename, data):
> > +        try:
> > +            ret = subprocess.run(['autopep8', '--ignore=E501', '-'],
> > +                                 input=data.encode('utf-8'), stdout=subprocess.PIPE)
> > +        except FileNotFoundError:
> > +            issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions'))
> > +            return issues
> > +
> > +        return ret.stdout.decode('utf-8')
> > +
> > +
> 
> Testing this, on a simple patch that breaks the coding style, I get
> 
> ----------------------------------------------------------------------------------
> dea6e2fea04866b477ca9bd622c885ca74b50d9c utils: checkstyle.py: Test PEP8 formatter
> ----------------------------------------------------------------------------------
> --- utils/checkstyle.py
> +++ utils/checkstyle.py
> @@ -728,7 +730,7 @@
>              issues.append(StyleIssue(0, None, None, 'Please install pycodestyle to validate python additions'))
>              return issues
> 
> -        results = ret.stdout.decode('utf-8').splitlines( )
> +        results = ret.stdout.decode('utf-8').splitlines()
>          for item in results:
>              search = re.search(Pep8Checker.results_regex, item)
>              line_number = int(search.group(1))
> #731: E201 whitespace after '('
> +        results = ret.stdout.decode('utf-8').splitlines( )
>                                                           ^
> ---
> 2 potential issues detected, please review
> 
> [master dea6e2fea048] utils: checkstyle.py: Test PEP8 formatter
> 
> 
> The checker and formatter generate duplicate issues. Would you consider
> than as an issue ? Should we disable the checker when the dependencies
> of the formatter are available ? Or could the checker report additional
> issues ?

Hm. I saw cases (E.g. only a single blank line before a class) where the
formatter (which added the missing line above the class) created a
change that didn't intersect with the modification, but the Issue was
correctly detected. For a moment I thought about adding a line above and
below in the hunk intersect function, but that felt wrong. Skipping the
checker would silence that issue, which is also not good. Adding more
logic just for python seems to be too much effort.  Maybe just leave the
duplicate issues, as we only do limited python development?

Regards,
Stefan

> 
> 
> >  # ------------------------------------------------------------------------------
> >  # Style checking
> >  #
> 
> -- 
> Regards,
> 
> Laurent Pinchart