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

[Max Reitz]

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

> 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?)

[...]

> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
>  def remote_filename(path):
>      if imgproto == 'file':
>          return path
> -    elif imgproto == 'ssh':
> +    if imgproto == 'ssh':

This seems like a weird thing to complain about for a coding style
check, but whatever.

(As in, I prefer the elif form)

>          return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
> -    else:
> -        raise Exception("Protocol %s not supported" % (imgproto))
> +    raise Exception("Protocol %s not supported" % (imgproto))
>  
>  class VM(qtest.QEMUQtestMachine):
>      '''A QEMU VM'''

[...]

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

[...]

> @@ -891,13 +892,14 @@ def wait_until_completed(self, drive='drive0', 
> check_offset=True, wait=60.0,
>                          self.assert_qmp(event, 'data/error', error)
>                      self.assert_no_active_block_jobs()
>                      return event
> -                elif event['event'] == 'JOB_STATUS_CHANGE':
> +                if event['event'] == 'JOB_STATUS_CHANGE':
>                      self.assert_qmp(event, 'data/id', drive)
>  
>      def wait_ready(self, drive='drive0'):
>          '''Wait until a block job BLOCK_JOB_READY event'''
> -        f = {'data': {'type': 'mirror', 'device': drive } }
> +        f = {'data': {'type': 'mirror', 'device': drive}}
>          event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
> +        return event

Why not just “return self.vm.event_wait…”?

Max

signature.asc
Description: OpenPGP digital signature