| Message ID | 20240830125311.1702053-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > # ------------------------------------------------------------------------------ > # Style checking > #
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@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
Hi Stefan, On Mon, Sep 02, 2024 at 04:14:20PM +0200, Stefan Klug wrote: > On Fri, Aug 30, 2024 at 06:06:28PM +0300, Laurent Pinchart wrote: > > 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@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? Do you plan to stop working on the tuning tool ? :-) If this is caused by an off-by-one but I'd rather fix it instead of carrying the annoyance of duplicated issues. Can you provide a sample commit that reproduces the problem ? > > > # ------------------------------------------------------------------------------ > > > # Style checking > > > #
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') + + # ------------------------------------------------------------------------------ # Style checking #
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@ideasonboard.com> --- utils/checkstyle.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+)