[{"id":30993,"web_url":"https://patchwork.libcamera.org/comment/30993/","msgid":"<20240830150628.GD4642@pendragon.ideasonboard.com>","date":"2024-08-30T15:06:28","subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Aug 30, 2024 at 02:53:00PM +0200, Stefan Klug wrote:\n> Reporting style issues on python files is great, automatically fixing\n> them is even better. Add a call to autopep8 for python files.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> utils/checkstyle.py | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n> \n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index 5901f1a71562..ed15145b64a4 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -954,6 +954,21 @@ class StripTrailingSpaceFormatter(Formatter):\n> return ''.join(lines)\n> \n> \n> +class Pep8Formatter(Formatter):\n> + patterns = ('*.py',)\n> +\n> + @classmethod\n> + def format(cls, filename, data):\n> + try:\n> + ret = subprocess.run(['autopep8', '--ignore=E501', '-'],\n> + input=data.encode('utf-8'), stdout=subprocess.PIPE)\n> + except FileNotFoundError:\n> + issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions'))\n> + return issues\n> +\n> + return ret.stdout.decode('utf-8')\n> +\n> +\n\nTesting this, on a simple patch that breaks the coding style, I get\n\n----------------------------------------------------------------------------------\ndea6e2fea04866b477ca9bd622c885ca74b50d9c utils: checkstyle.py: Test PEP8 formatter\n----------------------------------------------------------------------------------\n--- utils/checkstyle.py\n+++ utils/checkstyle.py\n@@ -728,7 +730,7 @@\n issues.append(StyleIssue(0, None, None, 'Please install pycodestyle to validate python additions'))\n return issues\n\n- results = ret.stdout.decode('utf-8').splitlines( )\n+ results = ret.stdout.decode('utf-8').splitlines()\n for item in results:\n search = re.search(Pep8Checker.results_regex, item)\n line_number = int(search.group(1))\n#731: E201 whitespace after '('\n+ results = ret.stdout.decode('utf-8').splitlines( )\n ^\n---\n2 potential issues detected, please review\n\n[master dea6e2fea048] utils: checkstyle.py: Test PEP8 formatter\n\n\nThe checker and formatter generate duplicate issues. Would you consider\nthan as an issue ? Should we disable the checker when the dependencies\nof the formatter are available ? Or could the checker report additional\nissues ?\n\n\n> # ------------------------------------------------------------------------------\n> # Style checking\n> #","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BC33DC323E\n\tfor ;\n\tFri, 30 Aug 2024 15:07:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE5896341E;\n\tFri, 30 Aug 2024 17:07:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C222861E4A\n\tfor ;\n\tFri, 30 Aug 2024 17:06:59 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 46A5C227;\n\tFri, 30 Aug 2024 17:05:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GakwZ2Zf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725030350;\n\tbh=xIuT0tuxuJktAc2OwPINt4rV4quZzHbgRBTCnPEfaRc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GakwZ2ZfbKy0/GMUO5WI35dVA3DAlxPKA4O9cvJZ8WFGFO7TNdGJO3jQYAgICcv+3\n\tDYBCTU0Q0NUhBAe39ylBPH4LlrOXcZg6BbHM6YSHAysWEGcmWrixh7OJLo4DxyvhXO\n\tY16eSYkCCUVxA3ka2ITbG5o6rNQjZrSQrTYb8ask=","Date":"Fri, 30 Aug 2024 18:06:28 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","Message-ID":"<20240830150628.GD4642@pendragon.ideasonboard.com>","References":"<20240830125311.1702053-1-stefan.klug@ideasonboard.com>\n\t<20240830125311.1702053-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240830125311.1702053-4-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31050,"web_url":"https://patchwork.libcamera.org/comment/31050/","msgid":"","date":"2024-09-02T14:14:20","subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Fri, Aug 30, 2024 at 06:06:28PM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 30, 2024 at 02:53:00PM +0200, Stefan Klug wrote:\n> > Reporting style issues on python files is great, automatically fixing\n> > them is even better. Add a call to autopep8 for python files.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > utils/checkstyle.py | 15 +++++++++++++++\n> > 1 file changed, 15 insertions(+)\n> > \n> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > index 5901f1a71562..ed15145b64a4 100755\n> > --- a/utils/checkstyle.py\n> > +++ b/utils/checkstyle.py\n> > @@ -954,6 +954,21 @@ class StripTrailingSpaceFormatter(Formatter):\n> > return ''.join(lines)\n> > \n> > \n> > +class Pep8Formatter(Formatter):\n> > + patterns = ('*.py',)\n> > +\n> > + @classmethod\n> > + def format(cls, filename, data):\n> > + try:\n> > + ret = subprocess.run(['autopep8', '--ignore=E501', '-'],\n> > + input=data.encode('utf-8'), stdout=subprocess.PIPE)\n> > + except FileNotFoundError:\n> > + issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions'))\n> > + return issues\n> > +\n> > + return ret.stdout.decode('utf-8')\n> > +\n> > +\n> \n> Testing this, on a simple patch that breaks the coding style, I get\n> \n> ----------------------------------------------------------------------------------\n> dea6e2fea04866b477ca9bd622c885ca74b50d9c utils: checkstyle.py: Test PEP8 formatter\n> ----------------------------------------------------------------------------------\n> --- utils/checkstyle.py\n> +++ utils/checkstyle.py\n> @@ -728,7 +730,7 @@\n> issues.append(StyleIssue(0, None, None, 'Please install pycodestyle to validate python additions'))\n> return issues\n> \n> - results = ret.stdout.decode('utf-8').splitlines( )\n> + results = ret.stdout.decode('utf-8').splitlines()\n> for item in results:\n> search = re.search(Pep8Checker.results_regex, item)\n> line_number = int(search.group(1))\n> #731: E201 whitespace after '('\n> + results = ret.stdout.decode('utf-8').splitlines( )\n> ^\n> ---\n> 2 potential issues detected, please review\n> \n> [master dea6e2fea048] utils: checkstyle.py: Test PEP8 formatter\n> \n> \n> The checker and formatter generate duplicate issues. Would you consider\n> than as an issue ? Should we disable the checker when the dependencies\n> of the formatter are available ? Or could the checker report additional\n> issues ?\n\nHm. I saw cases (E.g. only a single blank line before a class) where the\nformatter (which added the missing line above the class) created a\nchange that didn't intersect with the modification, but the Issue was\ncorrectly detected. For a moment I thought about adding a line above and\nbelow in the hunk intersect function, but that felt wrong. Skipping the\nchecker would silence that issue, which is also not good. Adding more\nlogic just for python seems to be too much effort. Maybe just leave the\nduplicate issues, as we only do limited python development?\n\nRegards,\nStefan\n\n> \n> \n> > # ------------------------------------------------------------------------------\n> > # Style checking\n> > #\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6258AC324C\n\tfor ;\n\tMon, 2 Sep 2024 14:14:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06F9E618FF;\n\tMon, 2 Sep 2024 16:14:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7EC7618FD\n\tfor ;\n\tMon, 2 Sep 2024 16:14:22 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:ed94:5d45:6ff6:1489])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2EE64CE;\n\tMon, 2 Sep 2024 16:13:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hWdk9p9n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725286391;\n\tbh=aqBJfLOQ0RnheeZet0YOekq8KyncYeDxT/YHka29wTY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hWdk9p9nDBg4cpXLZWxZs+taNgzZUqR+IUj4OxDvmMMA0GYdd6nni/Y4hruRu2Ige\n\tDgYrTZVvDOUaIQp4ZJSftP8u20xEXSRIOlX6YkPlUhtUw8I5b2mIHYzkJMBO6ot/l9\n\tx533Qe7nwsJw0LIitzSv56p+/3vBAe7SpGwPJQlE=","Date":"Mon, 2 Sep 2024 16:14:20 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","Message-ID":"","References":"<20240830125311.1702053-1-stefan.klug@ideasonboard.com>\n\t<20240830125311.1702053-4-stefan.klug@ideasonboard.com>\n\t<20240830150628.GD4642@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240830150628.GD4642@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31051,"web_url":"https://patchwork.libcamera.org/comment/31051/","msgid":"<20240902144134.GH4313@pendragon.ideasonboard.com>","date":"2024-09-02T14:41:34","subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nOn Mon, Sep 02, 2024 at 04:14:20PM +0200, Stefan Klug wrote:\n> On Fri, Aug 30, 2024 at 06:06:28PM +0300, Laurent Pinchart wrote:\n> > On Fri, Aug 30, 2024 at 02:53:00PM +0200, Stefan Klug wrote:\n> > > Reporting style issues on python files is great, automatically fixing\n> > > them is even better. Add a call to autopep8 for python files.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > utils/checkstyle.py | 15 +++++++++++++++\n> > > 1 file changed, 15 insertions(+)\n> > > \n> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > index 5901f1a71562..ed15145b64a4 100755\n> > > --- a/utils/checkstyle.py\n> > > +++ b/utils/checkstyle.py\n> > > @@ -954,6 +954,21 @@ class StripTrailingSpaceFormatter(Formatter):\n> > > return ''.join(lines)\n> > > \n> > > \n> > > +class Pep8Formatter(Formatter):\n> > > + patterns = ('*.py',)\n> > > +\n> > > + @classmethod\n> > > + def format(cls, filename, data):\n> > > + try:\n> > > + ret = subprocess.run(['autopep8', '--ignore=E501', '-'],\n> > > + input=data.encode('utf-8'), stdout=subprocess.PIPE)\n> > > + except FileNotFoundError:\n> > > + issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions'))\n> > > + return issues\n> > > +\n> > > + return ret.stdout.decode('utf-8')\n> > > +\n> > > +\n> > \n> > Testing this, on a simple patch that breaks the coding style, I get\n> > \n> > ----------------------------------------------------------------------------------\n> > dea6e2fea04866b477ca9bd622c885ca74b50d9c utils: checkstyle.py: Test PEP8 formatter\n> > ----------------------------------------------------------------------------------\n> > --- utils/checkstyle.py\n> > +++ utils/checkstyle.py\n> > @@ -728,7 +730,7 @@\n> > issues.append(StyleIssue(0, None, None, 'Please install pycodestyle to validate python additions'))\n> > return issues\n> > \n> > - results = ret.stdout.decode('utf-8').splitlines( )\n> > + results = ret.stdout.decode('utf-8').splitlines()\n> > for item in results:\n> > search = re.search(Pep8Checker.results_regex, item)\n> > line_number = int(search.group(1))\n> > #731: E201 whitespace after '('\n> > + results = ret.stdout.decode('utf-8').splitlines( )\n> > ^\n> > ---\n> > 2 potential issues detected, please review\n> > \n> > [master dea6e2fea048] utils: checkstyle.py: Test PEP8 formatter\n> > \n> > \n> > The checker and formatter generate duplicate issues. Would you consider\n> > than as an issue ? Should we disable the checker when the dependencies\n> > of the formatter are available ? Or could the checker report additional\n> > issues ?\n> \n> Hm. I saw cases (E.g. only a single blank line before a class) where the\n> formatter (which added the missing line above the class) created a\n> change that didn't intersect with the modification, but the Issue was\n> correctly detected. For a moment I thought about adding a line above and\n> below in the hunk intersect function, but that felt wrong. Skipping the\n> checker would silence that issue, which is also not good. Adding more\n> logic just for python seems to be too much effort. Maybe just leave the\n> duplicate issues, as we only do limited python development?\n\nDo you plan to stop working on the tuning tool ? :-) If this is caused\nby an off-by-one but I'd rather fix it instead of carrying the annoyance\nof duplicated issues. Can you provide a sample commit that reproduces\nthe problem ?\n\n> > > # ------------------------------------------------------------------------------\n> > > # Style checking\n> > > #","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B1EE5BF415\n\tfor ;\n\tMon, 2 Sep 2024 14:42:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6FC4634CB;\n\tMon, 2 Sep 2024 16:42:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C27D2618FD\n\tfor ;\n\tMon, 2 Sep 2024 16:42:05 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 567034CE;\n\tMon, 2 Sep 2024 16:40:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lKkFrEhm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725288054;\n\tbh=Wy3fvn65B99VIwHvLzqTBsLtQJlTh4ZW6CXwn58OSBo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lKkFrEhmJTHnnw3bY69Zr42BOlO9a3+yos6Ju9RvKN4wr66d4fOfXLozvHdHfSHdW\n\t1BI01NlKHjL77R7W0eQ2JMKB/7zOwQ6D3vsktDpY1eLVtYkXAATHugGjwuI7sFTadv\n\t3HDZY6pYLTiGFC2DjZpUFLNuiXEPDcgwhR49L0ms=","Date":"Mon, 2 Sep 2024 17:41:34 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/3] utils: checkstyle: Add a python formatter","Message-ID":"<20240902144134.GH4313@pendragon.ideasonboard.com>","References":"<20240830125311.1702053-1-stefan.klug@ideasonboard.com>\n\t<20240830125311.1702053-4-stefan.klug@ideasonboard.com>\n\t<20240830150628.GD4642@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]