| Message ID | 568CFA8B.8080306@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
I guess this patch is not required, once Russel's patches on - Python style fixes and flake8 integration are completely merged. Thanks Numan On 01/06/2016 04:59 PM, Numan Siddique wrote: > with the flake8 check enabled, ovs compilation is failing. This > patch adds few more flake8 types to the igore list. > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- > Makefile.am | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile.am b/Makefile.am > index 8b6ddb7..cb73ca6 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -349,7 +349,7 @@ ALL_LOCAL += flake8-check > # E129 visually indented line with same indent as next logical line > # E131 continuation line unaligned for hanging indent > flake8-check: $(FLAKE8_PYFILES) > - $(AM_V_GEN) if flake8 $^ --ignore=E123,E126,E127,E128,E129,E131 ${FLAKE8_FLAGS}; then touch $@; else exit 1; fi > + $(AM_V_GEN) if flake8 $^ --ignore=E123,E126,E127,E128,E129,E131,D100,D101,D102,D103,D104,D105,D200,D202,D204,D205,D207,D208,D209,D210,D400,D401,H104,H201,H231,H232,H233,H238,H301,H306,H401,H403,H404,H405 ${FLAKE8_FLAGS}; then touch $@; else exit 1; fi > endif > > include $(srcdir)/manpages.mk
On 01/06/2016 06:47 AM, Numan Siddique wrote: > I guess this patch is not required, once Russel's patches on - Python style fixes and flake8 integration > are completely merged. They are all merged now. What version of flake8 do you have? I had this originally: $ flake8 --version 2.4.1 (pep8: 1.5.7, pyflakes: 0.8.1, mccabe: 0.3.1) CPython 2.7.10 on Linux I just upgraded and still don't see any new warnings: $ flake8 --version 2.5.1 (pep8: 1.5.7, pyflakes: 1.0.0, mccabe: 0.3.1) CPython 2.7.10 on Linux I installed via pip. $ sudo pip install flake8 or to upgrade via pip, run: $ sudo pip install -U flake8 Assuming it's just a version issue, we have a couple of choices: 1) Update the configure script to ensure some minimum version of flake8. 2) Go back to how I originally implemented this, which was to run flake8 via 'tox', which ensures everyone uses the same version by automatically creating/using a Python virtualenv and packages from PyPI. I think we should just try #1 unless this turns out to be painful for some reason.
On 01/06/2016 07:46 PM, Russell Bryant wrote: > On 01/06/2016 06:47 AM, Numan Siddique wrote: >> I guess this patch is not required, once Russel's patches on - Python style fixes and flake8 integration >> are completely merged. > They are all merged now. What version of flake8 do you have? > > I had this originally: > > $ flake8 --version > 2.4.1 (pep8: 1.5.7, pyflakes: 0.8.1, mccabe: 0.3.1) CPython 2.7.10 on Linux > > I just upgraded and still don't see any new warnings: > > $ flake8 --version > 2.5.1 (pep8: 1.5.7, pyflakes: 1.0.0, mccabe: 0.3.1) CPython 2.7.10 on Linux > > I installed via pip. > > $ sudo pip install flake8 > > or to upgrade via pip, run: > > $ sudo pip install -U flake8 > > > Assuming it's just a version issue, we have a couple of choices: > > 1) Update the configure script to ensure some minimum version of flake8. > > 2) Go back to how I originally implemented this, which was to run flake8 > via 'tox', which ensures everyone uses the same version by automatically > creating/using a Python virtualenv and packages from PyPI. > > I think we should just try #1 unless this turns out to be painful for > some reason. > I have 2.2.4 version. I will upgrade flake8 to latest version and test. I also see the failures in the openstack CI for networking-ovn for one of the patch [1] [1] - http://logs.openstack.org/26/178826/18/check/gate-tempest-dsvm-networking-ovn/cb05603/logs/devstacklog.txt.gz https://review.openstack.org/#/c/178826/ Thanks Numan
On 01/06/2016 09:23 AM, Numan Siddique wrote: > On 01/06/2016 07:46 PM, Russell Bryant wrote: >> On 01/06/2016 06:47 AM, Numan Siddique wrote: >>> I guess this patch is not required, once Russel's patches on - Python style fixes and flake8 integration >>> are completely merged. >> They are all merged now. What version of flake8 do you have? >> >> I had this originally: >> >> $ flake8 --version >> 2.4.1 (pep8: 1.5.7, pyflakes: 0.8.1, mccabe: 0.3.1) CPython 2.7.10 on Linux >> >> I just upgraded and still don't see any new warnings: >> >> $ flake8 --version >> 2.5.1 (pep8: 1.5.7, pyflakes: 1.0.0, mccabe: 0.3.1) CPython 2.7.10 on Linux >> >> I installed via pip. >> >> $ sudo pip install flake8 >> >> or to upgrade via pip, run: >> >> $ sudo pip install -U flake8 >> >> >> Assuming it's just a version issue, we have a couple of choices: >> >> 1) Update the configure script to ensure some minimum version of flake8. >> >> 2) Go back to how I originally implemented this, which was to run flake8 >> via 'tox', which ensures everyone uses the same version by automatically >> creating/using a Python virtualenv and packages from PyPI. >> >> I think we should just try #1 unless this turns out to be painful for >> some reason. >> > > I have 2.2.4 version. I will upgrade flake8 to latest version and test. > I also see the failures in the openstack CI for networking-ovn for one of the patch [1] > > [1] - http://logs.openstack.org/26/178826/18/check/gate-tempest-dsvm-networking-ovn/cb05603/logs/devstacklog.txt.gz > https://review.openstack.org/#/c/178826/ Yes, I just saw that too. I'll go ahead and update the configure check to ensure a newer version.
On 01/06/2016 06:29 AM, Numan Siddique wrote: > with the flake8 check enabled, ovs compilation is failing. This > patch adds few more flake8 types to the igore list. > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Thanks for the patch! After some discussion on IRC and more testing, I pushed this to master with some minor additions. The issue turned out to be flake8 plugins. My laptop did not have the docstrings and hacking flake8 plugins, which generate these additional errors. Unfortunately, I don't see a way to configure flake8 with an explicit list of plugins to use, so we just get whatever is installed. If these system differences continue to be painful, the cleanest solution would be to switch back to using 'tox', which will run flake8 in a consistently created virtual environment. The biggest downside is that it requires internet access to download packages when creating the virtualenv, and I don't think that requirement exists for any other part of the ovs build or test setup.
On Wed, Jan 06, 2016 at 10:19:56AM -0500, Russell Bryant wrote: > On 01/06/2016 06:29 AM, Numan Siddique wrote: > > with the flake8 check enabled, ovs compilation is failing. This > > patch adds few more flake8 types to the igore list. > > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > Thanks for the patch! After some discussion on IRC and more testing, I > pushed this to master with some minor additions. > > The issue turned out to be flake8 plugins. My laptop did not have the > docstrings and hacking flake8 plugins, which generate these additional > errors. Unfortunately, I don't see a way to configure flake8 with an > explicit list of plugins to use, so we just get whatever is installed. Would it work better to whitelist the warnings we want, with --select, instead of blacklisting the warnings we don't want with --ignore?
On 01/06/2016 11:32 AM, Ben Pfaff wrote: > On Wed, Jan 06, 2016 at 10:19:56AM -0500, Russell Bryant wrote: >> On 01/06/2016 06:29 AM, Numan Siddique wrote: >>> with the flake8 check enabled, ovs compilation is failing. This >>> patch adds few more flake8 types to the igore list. >>> >>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >> >> Thanks for the patch! After some discussion on IRC and more testing, I >> pushed this to master with some minor additions. >> >> The issue turned out to be flake8 plugins. My laptop did not have the >> docstrings and hacking flake8 plugins, which generate these additional >> errors. Unfortunately, I don't see a way to configure flake8 with an >> explicit list of plugins to use, so we just get whatever is installed. > > Would it work better to whitelist the warnings we want, with --select, > instead of blacklisting the warnings we don't want with --ignore? > Maybe ... I'm not sure how to get a list. It could be hundreds of entries long. We also wouldn't get new ones as they get added in upgrades. I was able to come up with a minor improvement. --ignore=E123,E126,E127,E128,E129,E131,W503,D,H This ignores all of D*** and H***. This would have been a little better, but the --select overrides the --ignore here, so it didn't behave how I wanted. --select=E,W,F --ignore=E123,E126,E127,E128,E129,E131,W503 I'll post a patch in a minute.
On Wed, Jan 06, 2016 at 11:50:15AM -0500, Russell Bryant wrote: > On 01/06/2016 11:32 AM, Ben Pfaff wrote: > > On Wed, Jan 06, 2016 at 10:19:56AM -0500, Russell Bryant wrote: > >> On 01/06/2016 06:29 AM, Numan Siddique wrote: > >>> with the flake8 check enabled, ovs compilation is failing. This > >>> patch adds few more flake8 types to the igore list. > >>> > >>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > >> > >> Thanks for the patch! After some discussion on IRC and more testing, I > >> pushed this to master with some minor additions. > >> > >> The issue turned out to be flake8 plugins. My laptop did not have the > >> docstrings and hacking flake8 plugins, which generate these additional > >> errors. Unfortunately, I don't see a way to configure flake8 with an > >> explicit list of plugins to use, so we just get whatever is installed. > > > > Would it work better to whitelist the warnings we want, with --select, > > instead of blacklisting the warnings we don't want with --ignore? > > Maybe ... I'm not sure how to get a list. It could be hundreds of > entries long. We also wouldn't get new ones as they get added in upgrades. > > I was able to come up with a minor improvement. > > --ignore=E123,E126,E127,E128,E129,E131,W503,D,H > > This ignores all of D*** and H***. > > This would have been a little better, but the --select overrides the > --ignore here, so it didn't behave how I wanted. > > --select=E,W,F --ignore=E123,E126,E127,E128,E129,E131,W503 > > I'll post a patch in a minute. OK. Thanks.
diff --git a/Makefile.am b/Makefile.am index 8b6ddb7..cb73ca6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -349,7 +349,7 @@ ALL_LOCAL += flake8-check # E129 visually indented line with same indent as next logical line # E131 continuation line unaligned for hanging indent flake8-check: $(FLAKE8_PYFILES) - $(AM_V_GEN) if flake8 $^ --ignore=E123,E126,E127,E128,E129,E131 ${FLAKE8_FLAGS}; then touch $@; else exit 1; fi + $(AM_V_GEN) if flake8 $^ --ignore=E123,E126,E127,E128,E129,E131,D100,D101,D102,D103,D104,D105,D200,D202,D204,D205,D207,D208,D209,D210,D400,D401,H104,H201,H231,H232,H233,H238,H301,H306,H401,H403,H404,H405 ${FLAKE8_FLAGS}; then touch $@; else exit 1; fi endif include $(srcdir)/manpages.mk
with the flake8 check enabled, ovs compilation is failing. This patch adds few more flake8 types to the igore list. Signed-off-by: Numan Siddique <nusiddiq@redhat.com> --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)