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