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

[John Snow <jsnow@redhat.com>]

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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  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.

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

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

Honestly, I do too. We can silence the warning instead.

This warning option doesn't like "if return else return" constructs,
preferring instead:

if x:
    return 0
return 1

but I have to admit that I often like to see the branches laid out as
branches, too.

Other Pythonistas (Eduardo, Philippe, Markus?) -- strong feelings one
way or the other?

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

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


>> @@ -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…”?
> 

Shrug. Sometimes I name my return variables when working in Python to
give some semantic clue over what exactly I'm even returning.

I can change it; but the docstring will grow to describe what it returns
to re-document the same.

--js