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

[Kevin Wolf]

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.

> >> 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...

> >> @@ -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.

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