4:55 a.m.
Now sc_prohibit_semicolon_at_eol_in_python can't handle semicolon
within multiline strings(comments) properly.
I suggest that we could use pep8 to check python code style error, such
as 'statement ends with a semicolon'. In future, we could use '--select'
to introduce other rules.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
cfg.mk | 6 ++----
configure.ac | 4 ++++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 42e1abf0..c8eaf74e 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -813,10 +813,8 @@ sc_require_enum_last_marker:
# In Python files we don't want to end lines with a semicolon like in C
sc_prohibit_semicolon_at_eol_in_python:
- @prohibit='^[^#].*\;$$' \
- in_vc_files='\.py$$' \
- halt='python does not require to end lines with a semicolon' \
- $(_sc_search_regexp)
+ @$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs $(PEP8) --select E703 \
+ | $(GREP) . && { exit 1; } || :
# mymain() in test files should use return, not exit, for nicer output
sc_prohibit_exit_in_tests:
diff --git a/configure.ac b/configure.ac
index bf9e7681..37fa9924 100644
--- a/configure.ac
+++ b/configure.ac
@@ -704,6 +704,10 @@ AC_PATH_PROGS([PYTHON], [python3 python2 python])
if test -z "$PYTHON"; then
AC_MSG_ERROR(['python3', 'python2' or 'python' binary is
required to build libvirt])
fi
+AC_PATH_PROG([PEP8], [pep8])
+if test -z "$PEP8"; then
+ AC_MSG_ERROR(['pep8' binary is required to check python code style])
+fi
AC_PATH_PROG([PERL], [perl])
if test -z "$PERL"; then
AC_MSG_ERROR(['perl' binary is required to build libvirt])
--
2.17.1
5:13 a.m.
On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
Now sc_prohibit_semicolon_at_eol_in_python can't handle
semicolon
within multiline strings(comments) properly.
I suggest that we could use pep8 to check python code style error, such
as 'statement ends with a semicolon'. In future, we could use '--select'
to introduce other rules.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
cfg.mk | 6 ++----
configure.ac | 4 ++++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 42e1abf0..c8eaf74e 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -813,10 +813,8 @@ sc_require_enum_last_marker:
# In Python files we don't want to end lines with a semicolon like in C
sc_prohibit_semicolon_at_eol_in_python:
- @prohibit='^[^#].*\;$$' \
- in_vc_files='\.py$$' \
- halt='python does not require to end lines with a semicolon' \
- $(_sc_search_regexp)
+ @$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs $(PEP8) --select E703 \
+ | $(GREP) . && { exit 1; } || :
# mymain() in test files should use return, not exit, for nicer output
sc_prohibit_exit_in_tests:
diff --git a/configure.ac b/configure.ac
index bf9e7681..37fa9924 100644
--- a/configure.ac
+++ b/configure.ac
@@ -704,6 +704,10 @@ AC_PATH_PROGS([PYTHON], [python3 python2 python])
if test -z "$PYTHON"; then
AC_MSG_ERROR(['python3', 'python2' or 'python' binary is
required to build libvirt])
fi
+AC_PATH_PROG([PEP8], [pep8])
+if test -z "$PEP8"; then
+ AC_MSG_ERROR(['pep8' binary is required to check python code style])
+fi
AC_PATH_PROG([PERL], [perl])
if test -z "$PERL"; then
AC_MSG_ERROR(['perl' binary is required to build libvirt])
Using pep8 is an interesting idea. Especially with my series to
standardize on using python for all build scripts, it will be
valuable to have much more advanced python style checks.
The only thing I wonder about is whether its reasonable to make
it a mandatory requirement or not, since it is a separate package
from python itself we can't assume it is present I think. It is
on the various Linux we care about and FreeBSD too, but I'm not
seeing it for macOS via homebrew.
Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
the python3 impl. Except that when I run it, it warns that
it is renamed to pycodestyle upstream and 'pep8' will be dropped
in future.
IOW, I think we'll need to check for existence of the first available
bniary from the list in this order:
pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:56 a.m.
On Thu, 2019-09-12 at 11:13 +0100, Daniel P. Berrangé wrote:
On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
> +AC_PATH_PROG([PEP8], [pep8])
> +if test -z "$PEP8"; then
> + AC_MSG_ERROR(['pep8' binary is required to check python code style])
> +fi
Using pep8 is an interesting idea. Especially with my series to
standardize on using python for all build scripts, it will be
valuable to have much more advanced python style checks.
The only thing I wonder about is whether its reasonable to make
it a mandatory requirement or not, since it is a separate package
from python itself we can't assume it is present I think. It is
on the various Linux we care about and FreeBSD too, but I'm not
seeing it for macOS via homebrew.
Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
the python3 impl. Except that when I run it, it warns that
it is renamed to pycodestyle upstream and 'pep8' will be dropped
in future.
IOW, I think we'll need to check for existence of the first available
bniary from the list in this order:
pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2
FWIW, libvirt-dbus is using flake8 to achieve what I believe is
basically the same result, whereas virt-manager I think uses pylint
and pycodestlye.
I am not familiar enough with the Python ecosystem to be able to
compare the various linters, but it would IMHO make sense to at
least try to standardize on one or more of them and use them across
libvirt-related projects.
CC'ing Cole and Pavel, who as maintainers of the biggest chunk of
Python code in the entire stack can probably offer some useful
insights.
--
Andrea Bolognani / Red Hat / Virtualization
6 a.m.
On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
On Thu, 2019-09-12 at 11:13 +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
> > +AC_PATH_PROG([PEP8], [pep8])
> > +if test -z "$PEP8"; then
> > + AC_MSG_ERROR(['pep8' binary is required to check python code
style])
> > +fi
>
> Using pep8 is an interesting idea. Especially with my series to
> standardize on using python for all build scripts, it will be
> valuable to have much more advanced python style checks.
>
> The only thing I wonder about is whether its reasonable to make
> it a mandatory requirement or not, since it is a separate package
> from python itself we can't assume it is present I think. It is
> on the various Linux we care about and FreeBSD too, but I'm not
> seeing it for macOS via homebrew.
>
> Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
> the python3 impl. Except that when I run it, it warns that
> it is renamed to pycodestyle upstream and 'pep8' will be dropped
> in future.
>
> IOW, I think we'll need to check for existence of the first available
> bniary from the list in this order:
>
> pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2
FWIW, libvirt-dbus is using flake8 to achieve what I believe is
basically the same result, whereas virt-manager I think uses pylint
and pycodestlye.
I am not familiar enough with the Python ecosystem to be able to
compare the various linters, but it would IMHO make sense to at
least try to standardize on one or more of them and use them across
libvirt-related projects.
pep8 validates code style against published PEP style guidelines.
pyflakes does static analysis to detect code errors
flake8 is a wrapper that runs pep8 and pyflakes and does some
other stuff.
For just doing this semicolon check then pep8 is sufficient,
but for a more general approach, then flake8 makes more
sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
entirely, and simply have a generic 'sc_flake8' check that runs a
configured list of checks against all py code.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:18 a.m.
On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
> basically the same result, whereas virt-manager I think uses pylint
> and pycodestlye.
>
> I am not familiar enough with the Python ecosystem to be able to
> compare the various linters, but it would IMHO make sense to at
> least try to standardize on one or more of them and use them across
> libvirt-related projects.
pep8 validates code style against published PEP style guidelines.
pyflakes does static analysis to detect code errors
flake8 is a wrapper that runs pep8 and pyflakes and does some
other stuff.
For just doing this semicolon check then pep8 is sufficient,
but for a more general approach, then flake8 makes more
sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
entirely, and simply have a generic 'sc_flake8' check that runs a
configured list of checks against all py code.
Yeah, the more general approach makes sense to me, so I would go
that route directly instead of introducing pep8 first and only then
moving to flake8.
--
Andrea Bolognani / Red Hat / Virtualization
9:05 a.m.
On 9/12/19 9:18 AM, Andrea Bolognani wrote:
On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
>> basically the same result, whereas virt-manager I think uses pylint
>> and pycodestlye.
>>
>> I am not familiar enough with the Python ecosystem to be able to
>> compare the various linters, but it would IMHO make sense to at
>> least try to standardize on one or more of them and use them across
>> libvirt-related projects.
>
> pep8 validates code style against published PEP style guidelines.
>
> pyflakes does static analysis to detect code errors
>
> flake8 is a wrapper that runs pep8 and pyflakes and does some
> other stuff.
>
> For just doing this semicolon check then pep8 is sufficient,
> but for a more general approach, then flake8 makes more
> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
> entirely, and simply have a generic 'sc_flake8' check that runs a
> configured list of checks against all py code.
Yeah, the more general approach makes sense to me, so I would go
that route directly instead of introducing pep8 first and only then
moving to flake8.
I don't have much experience with flake8 but I know it's commonly used,
so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
the project was renamed to pycodestyle in early 2016, which does affect
config file naming and format
https://github.com/PyCQA/pycodestyle
- Cole
10:47 a.m.
On Thu, Sep 12, 2019 at 10:05:34AM -0400, Cole Robinson wrote:
On 9/12/19 9:18 AM, Andrea Bolognani wrote:
> On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
>> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
>>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
>>> basically the same result, whereas virt-manager I think uses pylint
>>> and pycodestlye.
>>>
>>> I am not familiar enough with the Python ecosystem to be able to
>>> compare the various linters, but it would IMHO make sense to at
>>> least try to standardize on one or more of them and use them across
>>> libvirt-related projects.
>>
>> pep8 validates code style against published PEP style guidelines.
>>
>> pyflakes does static analysis to detect code errors
>>
>> flake8 is a wrapper that runs pep8 and pyflakes and does some
>> other stuff.
>>
>> For just doing this semicolon check then pep8 is sufficient,
>> but for a more general approach, then flake8 makes more
>> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
>> entirely, and simply have a generic 'sc_flake8' check that runs a
>> configured list of checks against all py code.
>
> Yeah, the more general approach makes sense to me, so I would go
> that route directly instead of introducing pep8 first and only then
> moving to flake8.
>
I don't have much experience with flake8 but I know it's commonly used,
so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
the project was renamed to pycodestyle in early 2016, which does affect
config file naming and format
https://github.com/PyCQA/pycodestyle
flake8 seems to know to use pycodestyle, so I'd say we should go
straight to flake8 and thus avoid worrying bout the pep/pycodestyle
rename.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:07 a.m.
On Thu, Sep 12, 2019 at 10:05:34AM -0400, Cole Robinson wrote:
> On 9/12/19 9:18 AM, Andrea Bolognani wrote:
> > On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
> >> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
> >>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
> >>> basically the same result, whereas virt-manager I think uses pylint
> >>> and pycodestlye.
> >>>
> >>> I am not familiar enough with the Python ecosystem to be able to
> >>> compare the various linters, but it would IMHO make sense to at
> >>> least try to standardize on one or more of them and use them across
> >>> libvirt-related projects.
> >>
> >> pep8 validates code style against published PEP style guidelines.
> >>
> >> pyflakes does static analysis to detect code errors
> >>
> >> flake8 is a wrapper that runs pep8 and pyflakes and does some
> >> other stuff.
> >>
> >> For just doing this semicolon check then pep8 is sufficient,
> >> but for a more general approach, then flake8 makes more
> >> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
> >> entirely, and simply have a generic 'sc_flake8' check that runs a
> >> configured list of checks against all py code.
> >
> > Yeah, the more general approach makes sense to me, so I would go
> > that route directly instead of introducing pep8 first and only then
> > moving to flake8.
> >
>
> I don't have much experience with flake8 but I know it's commonly used,
> so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
> the project was renamed to pycodestyle in early 2016, which does affect
> config file naming and format
>
> https://github.com/PyCQA/pycodestyle
flake8 seems to know to use pycodestyle, so I'd say we should go
straight to flake8 and thus avoid worrying bout the pep/pycodestyle
rename.
Regards,
Daniel
--
Ok. I try to provide another patch to remove sc_prohibit_semicolon_at_eol_in_python
and add a new 'sc_flake8' check.
Thanks,
Shi Lei
1898
days inactive
1898
days old
7 comments
5 participants
participants (5)
-
Andrea Bolognani -
Cole Robinson -
Daniel P. Berrangé -
Shi Lei -
shi_lei＠massclouds.com