Message ID | 20190704093502.27448-1-kieran.bingham@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
On 04/07/2019 10:35, Kieran Bingham wrote: > Process python additions with pep8 and report any errors that are added. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > utils/checkstyle.py | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > This checker allows the checkstyle script to self-identify any issue. > Isn't that recursivly fun? > > > Todo: > - This should not fail if pep8 is not present, and should instead report an > issue that pep8 is not installed. For reference, this was actually easy to add. The following is now in v2, but I'll await any further comments before reposting. > - ret = subprocess.run(['pep8', '--ignore=E501', '-'], > - input=self.__data, stdout=subprocess.PIPE) > + try: > + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > + input=self.__data, stdout=subprocess.PIPE) > + except FileNotFoundError: > + issues.append(StyleIssue(0, "", "Please install pep8 to validate python additions")) > + return issues > - The line_no_regex should ideally be compiled once per class, or globally? > But how then do you access a class object from the class instance... > - All of the pep8 failures in checkstyle.py should be fixed up ... > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py > index fab4b116d2ff..a960affc71a9 100755 > --- a/utils/checkstyle.py > +++ b/utils/checkstyle.py > @@ -276,6 +276,35 @@ class MesonChecker(StyleChecker): > return issues > > > +class Pep8Checker(StyleChecker): > + patterns = ('*.py',) > + > + def __init__(self, content): > + super().__init__() > + self.__content = content > + self.__data = "".join(content).encode('utf-8') > + > + def check(self, line_numbers): > + issues = [] > + line_no_regex = re.compile('stdin:([0-9]+):([0-9]+)(.*)') > + > + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > + input=self.__data, stdout=subprocess.PIPE) > + > + results = ret.stdout.decode('utf-8').splitlines() > + for item in results: > + search = re.search(line_no_regex, item) > + line_number = int(search.group(1)) > + position = int(search.group(2)) > + msg = search.group(3) > + > + if line_number in line_numbers: > + line = self.__content[line_number - 1] > + issues.append(StyleIssue(line_number, line, msg)) > + > + return issues > + > + > # ------------------------------------------------------------------------------ > # Formatters > # >
Hi Kieran, Thank you for the patch. On Thu, Jul 04, 2019 at 10:50:07AM +0100, Kieran Bingham wrote: > On 04/07/2019 10:35, Kieran Bingham wrote: > > Process python additions with pep8 and report any errors that are added. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > utils/checkstyle.py | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > This checker allows the checkstyle script to self-identify any issue. > > Isn't that recursivly fun? > > > > > > Todo: > > - This should not fail if pep8 is not present, and should instead report an > > issue that pep8 is not installed. > > For reference, this was actually easy to add. > > The following is now in v2, but I'll await any further comments before > reposting. > > > - ret = subprocess.run(['pep8', '--ignore=E501', '-'], > > - input=self.__data, stdout=subprocess.PIPE) > > + try: > > + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > > + input=self.__data, stdout=subprocess.PIPE) > > + except FileNotFoundError: > > + issues.append(StyleIssue(0, "", "Please install pep8 to validate python additions")) > > + return issues How about doing it the same way we handle the astyle and clang-format dependencies, in the main function ? > > - The line_no_regex should ideally be compiled once per class, or globally? > > But how then do you access a class object from the class instance... > > - All of the pep8 failures in checkstyle.py should be fixed up ... > > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py > > index fab4b116d2ff..a960affc71a9 100755 > > --- a/utils/checkstyle.py > > +++ b/utils/checkstyle.py > > @@ -276,6 +276,35 @@ class MesonChecker(StyleChecker): > > return issues > > > > > > +class Pep8Checker(StyleChecker): > > + patterns = ('*.py',) > > + > > + def __init__(self, content): > > + super().__init__() > > + self.__content = content > > + self.__data = "".join(content).encode('utf-8') s/""/''/ It makes not difference in practice, but the script tends to use single quotes, so let's be consistent. There's also no need to store __data in the class, you can make it a local variable where used. > > + > > + def check(self, line_numbers): > > + issues = [] > > + line_no_regex = re.compile('stdin:([0-9]+):([0-9]+)(.*)') You can move this to the class level, below patterns, and use it with Pep8Checker.line_no_regex. > > + > > + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > > + input=self.__data, stdout=subprocess.PIPE) > > + > > + results = ret.stdout.decode('utf-8').splitlines() > > + for item in results: > > + search = re.search(line_no_regex, item) > > + line_number = int(search.group(1)) > > + position = int(search.group(2)) > > + msg = search.group(3) > > + > > + if line_number in line_numbers: > > + line = self.__content[line_number - 1] > > + issues.append(StyleIssue(line_number, line, msg)) > > + > > + return issues > > + > > + > > # ------------------------------------------------------------------------------ > > # Formatters > > #
Hi Laurent, On 04/07/2019 12:56, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Jul 04, 2019 at 10:50:07AM +0100, Kieran Bingham wrote: >> On 04/07/2019 10:35, Kieran Bingham wrote: >>> Process python additions with pep8 and report any errors that are added. >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> utils/checkstyle.py | 29 +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> This checker allows the checkstyle script to self-identify any issue. >>> Isn't that recursivly fun? >>> >>> >>> Todo: >>> - This should not fail if pep8 is not present, and should instead report an >>> issue that pep8 is not installed. >> >> For reference, this was actually easy to add. >> >> The following is now in v2, but I'll await any further comments before >> reposting. >> >>> - ret = subprocess.run(['pep8', '--ignore=E501', '-'], >>> - input=self.__data, stdout=subprocess.PIPE) >>> + try: >>> + ret = subprocess.run(['pep8', '--ignore=E501', '-'], >>> + input=self.__data, stdout=subprocess.PIPE) >>> + except FileNotFoundError: >>> + issues.append(StyleIssue(0, "", "Please install pep8 to validate python additions")) >>> + return issues > > How about doing it the same way we handle the astyle and clang-format > dependencies, in the main function ? Hrm, that only gives me a way to declare pep8 as required or not required. I think pep8 should be required if someone edits any .py file in the project, but not otherwise ... any suggestions? >>> - The line_no_regex should ideally be compiled once per class, or globally? >>> But how then do you access a class object from the class instance... >>> - All of the pep8 failures in checkstyle.py should be fixed up ... >>> >>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py >>> index fab4b116d2ff..a960affc71a9 100755 >>> --- a/utils/checkstyle.py >>> +++ b/utils/checkstyle.py >>> @@ -276,6 +276,35 @@ class MesonChecker(StyleChecker): >>> return issues >>> >>> >>> +class Pep8Checker(StyleChecker): >>> + patterns = ('*.py',) >>> + >>> + def __init__(self, content): >>> + super().__init__() >>> + self.__content = content >>> + self.__data = "".join(content).encode('utf-8') > > s/""/''/ > > It makes not difference in practice, but the script tends to use single > quotes, so let's be consistent. Sure. > > There's also no need to store __data in the class, you can make it a > local variable where used. Moved. > >>> + >>> + def check(self, line_numbers): >>> + issues = [] >>> + line_no_regex = re.compile('stdin:([0-9]+):([0-9]+)(.*)') > > You can move this to the class level, below patterns, and use it with > Pep8Checker.line_no_regex. Done, thanks - That was simpler than I realised :D > >>> + >>> + ret = subprocess.run(['pep8', '--ignore=E501', '-'], >>> + input=self.__data, stdout=subprocess.PIPE) >>> + >>> + results = ret.stdout.decode('utf-8').splitlines() >>> + for item in results: >>> + search = re.search(line_no_regex, item) >>> + line_number = int(search.group(1)) >>> + position = int(search.group(2)) >>> + msg = search.group(3) >>> + >>> + if line_number in line_numbers: >>> + line = self.__content[line_number - 1] >>> + issues.append(StyleIssue(line_number, line, msg)) >>> + >>> + return issues >>> + >>> + >>> # ------------------------------------------------------------------------------ >>> # Formatters >>> # >
Hi Kieran, On Thu, Jul 04, 2019 at 02:29:58PM +0100, Kieran Bingham wrote: > On 04/07/2019 12:56, Laurent Pinchart wrote: > > On Thu, Jul 04, 2019 at 10:50:07AM +0100, Kieran Bingham wrote: > >> On 04/07/2019 10:35, Kieran Bingham wrote: > >>> Process python additions with pep8 and report any errors that are added. > >>> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> utils/checkstyle.py | 29 +++++++++++++++++++++++++++++ > >>> 1 file changed, 29 insertions(+) > >>> > >>> This checker allows the checkstyle script to self-identify any issue. > >>> Isn't that recursivly fun? > >>> > >>> > >>> Todo: > >>> - This should not fail if pep8 is not present, and should instead report an > >>> issue that pep8 is not installed. > >> > >> For reference, this was actually easy to add. > >> > >> The following is now in v2, but I'll await any further comments before > >> reposting. > >> > >>> - ret = subprocess.run(['pep8', '--ignore=E501', '-'], > >>> - input=self.__data, stdout=subprocess.PIPE) > >>> + try: > >>> + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > >>> + input=self.__data, stdout=subprocess.PIPE) > >>> + except FileNotFoundError: > >>> + issues.append(StyleIssue(0, "", "Please install pep8 to validate python additions")) > >>> + return issues > > > > How about doing it the same way we handle the astyle and clang-format > > dependencies, in the main function ? > > Hrm, that only gives me a way to declare pep8 as required or not required. > > I think pep8 should be required if someone edits any .py file in the > project, but not otherwise ... > > any suggestions? That's a good point. Let's keep the above code then, and refactor it later if more checkers or formatters require dependencies. Ideally we should extract a list of all the files that need to be touched, and print a warning for unmet dependencies for those files only. > >>> - The line_no_regex should ideally be compiled once per class, or globally? > >>> But how then do you access a class object from the class instance... > >>> - All of the pep8 failures in checkstyle.py should be fixed up ... > >>> > >>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py > >>> index fab4b116d2ff..a960affc71a9 100755 > >>> --- a/utils/checkstyle.py > >>> +++ b/utils/checkstyle.py > >>> @@ -276,6 +276,35 @@ class MesonChecker(StyleChecker): > >>> return issues > >>> > >>> > >>> +class Pep8Checker(StyleChecker): > >>> + patterns = ('*.py',) > >>> + > >>> + def __init__(self, content): > >>> + super().__init__() > >>> + self.__content = content > >>> + self.__data = "".join(content).encode('utf-8') > > > > s/""/''/ > > > > It makes not difference in practice, but the script tends to use single > > quotes, so let's be consistent. > > Sure. > > > There's also no need to store __data in the class, you can make it a > > local variable where used. > > Moved. > > >>> + > >>> + def check(self, line_numbers): > >>> + issues = [] > >>> + line_no_regex = re.compile('stdin:([0-9]+):([0-9]+)(.*)') > > > > You can move this to the class level, below patterns, and use it with > > Pep8Checker.line_no_regex. > > Done, thanks - That was simpler than I realised :D > > >>> + > >>> + ret = subprocess.run(['pep8', '--ignore=E501', '-'], > >>> + input=self.__data, stdout=subprocess.PIPE) > >>> + > >>> + results = ret.stdout.decode('utf-8').splitlines() > >>> + for item in results: > >>> + search = re.search(line_no_regex, item) > >>> + line_number = int(search.group(1)) > >>> + position = int(search.group(2)) > >>> + msg = search.group(3) > >>> + > >>> + if line_number in line_numbers: > >>> + line = self.__content[line_number - 1] > >>> + issues.append(StyleIssue(line_number, line, msg)) > >>> + > >>> + return issues > >>> + > >>> + > >>> # ------------------------------------------------------------------------------ > >>> # Formatters > >>> #
diff --git a/utils/checkstyle.py b/utils/checkstyle.py index fab4b116d2ff..a960affc71a9 100755 --- a/utils/checkstyle.py +++ b/utils/checkstyle.py @@ -276,6 +276,35 @@ class MesonChecker(StyleChecker): return issues +class Pep8Checker(StyleChecker): + patterns = ('*.py',) + + def __init__(self, content): + super().__init__() + self.__content = content + self.__data = "".join(content).encode('utf-8') + + def check(self, line_numbers): + issues = [] + line_no_regex = re.compile('stdin:([0-9]+):([0-9]+)(.*)') + + ret = subprocess.run(['pep8', '--ignore=E501', '-'], + input=self.__data, stdout=subprocess.PIPE) + + results = ret.stdout.decode('utf-8').splitlines() + for item in results: + search = re.search(line_no_regex, item) + line_number = int(search.group(1)) + position = int(search.group(2)) + msg = search.group(3) + + if line_number in line_numbers: + line = self.__content[line_number - 1] + issues.append(StyleIssue(line_number, line, msg)) + + return issues + + # ------------------------------------------------------------------------------ # Formatters #
Process python additions with pep8 and report any errors that are added. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- utils/checkstyle.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) This checker allows the checkstyle script to self-identify any issue. Isn't that recursivly fun? Todo: - This should not fail if pep8 is not present, and should instead report an issue that pep8 is not installed. - The line_no_regex should ideally be compiled once per class, or globally? But how then do you access a class object from the class instance... - All of the pep8 failures in checkstyle.py should be fixed up ...