[libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check

Erik Skultety posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ea91edf4a22516de09a6a1e1b1924e6ee759228d.1590411309.git.eskultet@redhat.com
scripts/check-remote-protocol.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
Posted by Erik Skultety 3 years, 10 months ago
With newer pycodestyle 2.6.0 (which is part of flake8-3.8.2) reports
the following pep violation during syntax-check:

../scripts/check-remote-protocol.py:95:9: E741 ambiguous variable name 'l'
    for l in err.strip().split("\n")

On all the distros we test on, this hasn't occurred yet, but with the
future update of flake8 it likely would. The fix is easy, just name the
variable appropriately.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---

The weird thing is that E741 checking has been in pycodestyle since 2.1.0 [1],
we now have 2.5.0 and yet only 2.6.0 is complaining about it
[1] https://pycodestyle.pycqa.org/en/latest/developer.html#id8

 scripts/check-remote-protocol.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
index 25caa19563..cf9e3f84a1 100644
--- a/scripts/check-remote-protocol.py
+++ b/scripts/check-remote-protocol.py
@@ -92,8 +92,8 @@ if out == "" or pdwtagsproc.returncode != 0:
     else:
         print("WARNING: exit code %d, pdwtags appears broken:" %
               pdwtagsproc.returncode, file=sys.stderr)
-    for l in err.strip().split("\n"):
-        print("WARNING: %s" % l, file=sys.stderr)
+    for line in err.strip().split("\n"):
+        print("WARNING: %s" % line, file=sys.stderr)
     print("WARNING: skipping the remote protocol test", file=sys.stderr)
     sys.exit(0)

--
2.25.4

Re: [libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
Posted by Michal Privoznik 3 years, 10 months ago
On 5/25/20 2:56 PM, Erik Skultety wrote:
> With newer pycodestyle 2.6.0 (which is part of flake8-3.8.2) reports
> the following pep violation during syntax-check:
> 
> ../scripts/check-remote-protocol.py:95:9: E741 ambiguous variable name 'l'
>      for l in err.strip().split("\n")
> 
> On all the distros we test on, this hasn't occurred yet, but with the
> future update of flake8 it likely would. The fix is easy, just name the
> variable appropriately.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> 
> The weird thing is that E741 checking has been in pycodestyle since 2.1.0 [1],
> we now have 2.5.0 and yet only 2.6.0 is complaining about it
> [1] https://pycodestyle.pycqa.org/en/latest/developer.html#id8
> 
>   scripts/check-remote-protocol.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
> index 25caa19563..cf9e3f84a1 100644
> --- a/scripts/check-remote-protocol.py
> +++ b/scripts/check-remote-protocol.py
> @@ -92,8 +92,8 @@ if out == "" or pdwtagsproc.returncode != 0:
>       else:
>           print("WARNING: exit code %d, pdwtags appears broken:" %
>                 pdwtagsproc.returncode, file=sys.stderr)
> -    for l in err.strip().split("\n"):
> -        print("WARNING: %s" % l, file=sys.stderr)
> +    for line in err.strip().split("\n"):
> +        print("WARNING: %s" % line, file=sys.stderr)
>       print("WARNING: skipping the remote protocol test", file=sys.stderr)
>       sys.exit(0)

Ah, the error is about short variable name, it's about 'l' looking too 
similar to 'I'  (well, if this is somebody's case then I say use better 
font if you want to code). But in order to shut the checker up:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
Posted by Ján Tomko 3 years, 10 months ago
On a Monday in 2020, Michal Privoznik wrote:
>On 5/25/20 2:56 PM, Erik Skultety wrote:
>>With newer pycodestyle 2.6.0 (which is part of flake8-3.8.2) reports
>>the following pep violation during syntax-check:
>>
>>../scripts/check-remote-protocol.py:95:9: E741 ambiguous variable name 'l'
>>     for l in err.strip().split("\n")
>>
>>On all the distros we test on, this hasn't occurred yet, but with the
>>future update of flake8 it likely would. The fix is easy, just name the
>>variable appropriately.
>>
>>Signed-off-by: Erik Skultety <eskultet@redhat.com>
>>---
>>
>>The weird thing is that E741 checking has been in pycodestyle since 2.1.0 [1],
>>we now have 2.5.0 and yet only 2.6.0 is complaining about it
>>[1] https://pycodestyle.pycqa.org/en/latest/developer.html#id8
>>
>>  scripts/check-remote-protocol.py | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
>>index 25caa19563..cf9e3f84a1 100644
>>--- a/scripts/check-remote-protocol.py
>>+++ b/scripts/check-remote-protocol.py
>>@@ -92,8 +92,8 @@ if out == "" or pdwtagsproc.returncode != 0:
>>      else:
>>          print("WARNING: exit code %d, pdwtags appears broken:" %
>>                pdwtagsproc.returncode, file=sys.stderr)
>>-    for l in err.strip().split("\n"):
>>-        print("WARNING: %s" % l, file=sys.stderr)
>>+    for line in err.strip().split("\n"):
>>+        print("WARNING: %s" % line, file=sys.stderr)
>>      print("WARNING: skipping the remote protocol test", file=sys.stderr)
>>      sys.exit(0)
>
>Ah, the error is about short variable name, it's about 'l' looking too 
>similar to 'I'  (well, if this is somebody's case then I say use 
>better font if you want to code). But in order to shut the checker up:
>

Note that we can also shut it up by adding it to our FLAKE8_IGNORE
in build-aux/syntax-check.mk, but I don't care either way.

Jano

>Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
>Michal
>
Re: [libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
Posted by Erik Skultety 3 years, 10 months ago
On Mon, May 25, 2020 at 06:17:06PM +0200, Ján Tomko wrote:
> On a Monday in 2020, Michal Privoznik wrote:
> > On 5/25/20 2:56 PM, Erik Skultety wrote:
> > > With newer pycodestyle 2.6.0 (which is part of flake8-3.8.2) reports
> > > the following pep violation during syntax-check:
> > >
> > > ../scripts/check-remote-protocol.py:95:9: E741 ambiguous variable name 'l'
> > >     for l in err.strip().split("\n")
> > >
> > > On all the distros we test on, this hasn't occurred yet, but with the
> > > future update of flake8 it likely would. The fix is easy, just name the
> > > variable appropriately.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > >
> > > The weird thing is that E741 checking has been in pycodestyle since 2.1.0 [1],
> > > we now have 2.5.0 and yet only 2.6.0 is complaining about it
> > > [1] https://pycodestyle.pycqa.org/en/latest/developer.html#id8
> > >
> > >  scripts/check-remote-protocol.py | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
> > > index 25caa19563..cf9e3f84a1 100644
> > > --- a/scripts/check-remote-protocol.py
> > > +++ b/scripts/check-remote-protocol.py
> > > @@ -92,8 +92,8 @@ if out == "" or pdwtagsproc.returncode != 0:
> > >      else:
> > >          print("WARNING: exit code %d, pdwtags appears broken:" %
> > >                pdwtagsproc.returncode, file=sys.stderr)
> > > -    for l in err.strip().split("\n"):
> > > -        print("WARNING: %s" % l, file=sys.stderr)
> > > +    for line in err.strip().split("\n"):
> > > +        print("WARNING: %s" % line, file=sys.stderr)
> > >      print("WARNING: skipping the remote protocol test", file=sys.stderr)
> > >      sys.exit(0)
> >
> > Ah, the error is about short variable name, it's about 'l' looking too
> > similar to 'I'  (well, if this is somebody's case then I say use better
> > font if you want to code). But in order to shut the checker up:

I won't blindly defend all the PEP guidelines, but they do exist for a reason
and just like we forbade usage of i,k in other that simple loop use cases, this
is a kind of similar on a global scale.

> >
>
> Note that we can also shut it up by adding it to our FLAKE8_IGNORE
> in build-aux/syntax-check.mk, but I don't care either way.

Of course we can and that was also on the table, but since it's soo trivial to
fix and it's also a good practice IMO, I went for a patch to the source instead.

Thanks for review.

Erik

Re: [libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
Posted by Michal Privoznik 3 years, 10 months ago
On 5/26/20 8:48 AM, Erik Skultety wrote:
> On Mon, May 25, 2020 at 06:17:06PM +0200, Ján Tomko wrote:
>> On a Monday in 2020, Michal Privoznik wrote:
>>> On 5/25/20 2:56 PM, Erik Skultety wrote:
>>>> With newer pycodestyle 2.6.0 (which is part of flake8-3.8.2) reports
>>>> the following pep violation during syntax-check:
>>>>
>>>> ../scripts/check-remote-protocol.py:95:9: E741 ambiguous variable name 'l'
>>>>      for l in err.strip().split("\n")
>>>>
>>>> On all the distros we test on, this hasn't occurred yet, but with the
>>>> future update of flake8 it likely would. The fix is easy, just name the
>>>> variable appropriately.
>>>>
>>>> Signed-off-by: Erik Skultety <eskultet@redhat.com>
>>>> ---
>>>>
>>>> The weird thing is that E741 checking has been in pycodestyle since 2.1.0 [1],
>>>> we now have 2.5.0 and yet only 2.6.0 is complaining about it
>>>> [1] https://pycodestyle.pycqa.org/en/latest/developer.html#id8
>>>>
>>>>   scripts/check-remote-protocol.py | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/check-remote-protocol.py b/scripts/check-remote-protocol.py
>>>> index 25caa19563..cf9e3f84a1 100644
>>>> --- a/scripts/check-remote-protocol.py
>>>> +++ b/scripts/check-remote-protocol.py
>>>> @@ -92,8 +92,8 @@ if out == "" or pdwtagsproc.returncode != 0:
>>>>       else:
>>>>           print("WARNING: exit code %d, pdwtags appears broken:" %
>>>>                 pdwtagsproc.returncode, file=sys.stderr)
>>>> -    for l in err.strip().split("\n"):
>>>> -        print("WARNING: %s" % l, file=sys.stderr)
>>>> +    for line in err.strip().split("\n"):
>>>> +        print("WARNING: %s" % line, file=sys.stderr)
>>>>       print("WARNING: skipping the remote protocol test", file=sys.stderr)
>>>>       sys.exit(0)
>>>
>>> Ah, the error is about short variable name, it's about 'l' looking too
>>> similar to 'I'  (well, if this is somebody's case then I say use better
>>> font if you want to code). But in order to shut the checker up:
> 
> I won't blindly defend all the PEP guidelines, but they do exist for a reason
> and just like we forbade usage of i,k in other that simple loop use cases, this
> is a kind of similar on a global scale.

I remember us forbiding 'ii', 'jj' and 'kk', not simple 'i', 'j' or 'k'. 
And we did so because the nested loops are then too messy. My point was 
that the PEP does not do the same like we do (forbid variables because 
of their ambiguous name), but because if you use wrong font then they 
might look the same (unless a variable named 'Iine' is introduced 😈)

Anyway, I'm okay with the change, for a C coder whose every Python 
script is still C with Python syntax, 'line' express it better what I 
get from 'strip().split("\n")'.

> 
>>>
>>
>> Note that we can also shut it up by adding it to our FLAKE8_IGNORE
>> in build-aux/syntax-check.mk, but I don't care either way.
> 
> Of course we can and that was also on the table, but since it's soo trivial to
> fix and it's also a good practice IMO, I went for a patch to the source instead.

Agreed.

Michal