Re: [libvirt PATCH] scripts: Fix E741 that pycodesyle is pointing out during syntax-check
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(a)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