Re: [PATCH v6 1/9] iotests: do a light delinting

[John Snow]

On 3/4/20 6:12 AM, Kevin Wolf wrote:
> Am 03.03.2020 um 22:25 hat John Snow geschrieben:
>>
>>
>> On 2/27/20 7:59 AM, Max Reitz wrote:
>>> On 27.02.20 01:06, John Snow wrote:
>>>> This doesn't fix everything in here, but it does help clean up the
>>>> pylint report considerably.
>>>>
>>>> This should be 100% style changes only; the intent is to make pylint
>>>> more useful by working on establishing a baseline for iotests that we
>>>> can gate against in the future. This will be important if (when?) we
>>>> begin adding type hints to our code base.
> 
> I'm not sure I understand this connection. mypy doesn't care about
> style.
> 

Each catches things the other doesn't -- and often getting mypy passing
involves new errors in non-type-checked contexts. I have personally
found it useful to *always use both tools*.

So having a pylint baseline will help ensure that -- while we transition
to mypy, and expect to see many errors during the transition -- we have
a solid baseline from the other tool to avoid regressions with.

>>>> Signed-off-by: John Snow <address@hidden>
>>>> ---
>>>>  tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
>>>>  1 file changed, 45 insertions(+), 43 deletions(-)
>>>
>>> I feel like I’m the wrongest person there is for reviewing a Python
>>> style-fixing patch, but here I am and so here I go:
>>
>> No, it's good.
> 
> Max can't be the wrongest person for it because that's already me.
> 
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index 8815052eb5..e8a0ea14fc 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>
>>> [...]
>>>
>>>> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>>>>                            ' '.join(qemu_nbd_args + ['--fork'] + 
>>>> list(args))))
>>>>      if exitcode == 0:
>>>>          return exitcode, ''
>>>> -    else:
>>>> -        return exitcode, subp.communicate()[0]
>>>> +    return exitcode, subp.communicate()[0]
>>>
>>> If we want to make such a change (which I don’t doubt we want), I think
>>> it should be the other way around: Make the condition “exitcode != 0”,
>>> so the final return is the return for the successful case.  (Just
>>> because I think that’s how we usually do it, at least in the qemu code?)
>>>
>>> [...]
>>>
>>
>> Yes, makes sense. I was behaving a little more mechanically.
> 
> Here and...
> 

This check can be disabled I think legitimately.

>>>> @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, 
>>>> expected_node, graph=None):
>>>>              assert node is not None, 'Cannot follow path %s%s' % (root, 
>>>> path)
>>>>  
>>>>              try:
>>>> -                node_id = next(edge['child'] for edge in graph['edges'] \
>>>> -                                             if edge['parent'] == 
>>>> node['id'] and
>>>> -                                                edge['name'] == 
>>>> child_name)
>>>> +                node_id = next(edge['child'] for edge in graph['edges']
>>>> +                               if edge['parent'] == node['id'] and
>>>> +                               edge['name'] == child_name)
>>>
>>> I don’t mind the if alignment, but I do mind not aligning the third line
>>> to the “edge” above it (i.e. the third line is part of the condition, so
>>> I’d align it to the “if” condition).
>>>
>>> But then again it’s nothing new that I like to disagree with commonly
>>> agreed-upon Python coding styles, so.
>>>
>>> [...]
>>>
>>
>> OK, that can be addressed by highlighting the sub-expression with
>> parentheses:
>>
>>         node_id = next(edge['child'] for edge in graph['edges']
>>                        if (edge['parent'] == node['id'] and
>>                            edge['name'] == child_name))
> 
> ...here I must say that while I think Max's suggestions feel like an
> improvement to me over the patch, I actually like the current code best
> in both cases.
> 

This check I think *should* stay enabled to catch bad indenting, even if
it comes out looking subpar (subjective) in this one instance.

We might be able to fine-tune the indent checks, but I think for one
debated instance ... it's not important.

> In fact, after scanning your patch, I feel it's actually the majority of
> changes that pylint wants that aren't an improvement... Maybe this just
> underlines the fact that I am the wrongest person to review such patches
> and not Max. Though I'm surprised that I'm generally not the person who
> introduces the code violating the rules, and I don't have the impression
> in this thread that anyone is eager to defend pylint's opinion.
> 
> Now I ran pylint myself and it prints some even more ridiculous warnings
> like variable names being too short for its liking. I guess this means
> that if we want to run it without warnings or errors, we need to use a
> config file anyway to disable the worst parts.
> 
> And if we have a config file anyway, maybe we can more selectively
> enable the checks that we actually want?
> 
> Kevin
> 

See patch 9 for said conf file. I did disable most of the silly ones.
With this patch (and a few others later on) pylint is silent and can be
used for gating.

I do think that's worth a few indent styles that you feel are suboptimal.

--js