Re: [PATCH v9 01/14] iotests: do a light delinting

[John Snow]

On 3/30/20 10:39 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> 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.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 83 ++-
>>  1 file changed, 43 insertions(+), 40 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7bc4934cd2..886ae962ae 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> [...]
>> @@ -1062,6 +1063,7 @@ def func_wrapper(test_case: QMPTestCase, *args, 
>> **kwargs):
>>  if usf_list:
>>  test_case.case_skip('{}: formats {} are not 
>> whitelisted'.format(
>>  test_case, usf_list))
>> +return None
>>  else:
>>  return func(test_case, *args, **kwargs)
>>  return func_wrapper
>> @@ -1073,6 +1075,7 @@ def skip_if_user_is_root(func):
>>  def func_wrapper(*args, **kwargs):
>>  if os.getuid() == 0:
>>  case_notrun('{}: cannot be run as root'.format(args[0]))
>> +return None
>>  else:
>>  return func(*args, **kwargs)
>>  return func_wrapper
> 
> Observation, not demand: this trades one kind of pylint report for
> another: inconsistent-return-statements for no-else-return.  PATCH 05
> suppresses no-else-return.
> 

Hm, yeah. I think there was some light consensus that "no-else-return"
was perfectly fine, so this patch builds towards that specific pylintrc.

It isn't, in my opinion, a regression or lateral movement as there isn't
a pylintrc baseline yet.

--js

Re: [PATCH v9 01/14] iotests: do a light delinting

[John Snow]

On 3/30/20 11:41 AM, Kevin Wolf wrote:
> Am 25.03.2020 um 00:20 hat John Snow geschrieben:
>> 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.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Max Reitz 
> 
>> @@ -550,8 +546,8 @@ def flatten_qmp_object(self, obj, output=None, 
>> basestr=''):
>>  if output is None:
>>  output = dict()
>>  if isinstance(obj, list):
>> -for i in range(len(obj)):
>> -self.flatten_qmp_object(obj[i], output, basestr + str(i) + 
>> '.')
>> +for i, atom in enumerate(obj):
>> +self.flatten_qmp_object(atom, output, basestr + str(i) + 
>> '.')
> 
> I think atom isn't strictly the right word because we expect nested data
> structures (as shown by the recursive call). If I understand correctly,
> what Python calls things in lists is "items".
> 

I can't imagine how the philosophers felt when they discovered subatomic
particles.

--js

Re: [PATCH v9 01/14] iotests: do a light delinting

[Kevin Wolf]

Am 25.03.2020 um 00:20 hat John Snow geschrieben:
> 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.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Max Reitz 

> @@ -550,8 +546,8 @@ def flatten_qmp_object(self, obj, output=None, 
> basestr=''):
>  if output is None:
>  output = dict()
>  if isinstance(obj, list):
> -for i in range(len(obj)):
> -self.flatten_qmp_object(obj[i], output, basestr + str(i) + 
> '.')
> +for i, atom in enumerate(obj):
> +self.flatten_qmp_object(atom, output, basestr + str(i) + '.')

I think atom isn't strictly the right word because we expect nested data
structures (as shown by the recursive call). If I understand correctly,
what Python calls things in lists is "items".

Kevin

Re: [PATCH v9 01/14] iotests: do a light delinting

[Markus Armbruster]

John Snow  writes:

> 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.
>
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py | 83 ++-
>  1 file changed, 43 insertions(+), 40 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7bc4934cd2..886ae962ae 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
[...]
> @@ -1062,6 +1063,7 @@ def func_wrapper(test_case: QMPTestCase, *args, 
> **kwargs):
>  if usf_list:
>  test_case.case_skip('{}: formats {} are not 
> whitelisted'.format(
>  test_case, usf_list))
> +return None
>  else:
>  return func(test_case, *args, **kwargs)
>  return func_wrapper
> @@ -1073,6 +1075,7 @@ def skip_if_user_is_root(func):
>  def func_wrapper(*args, **kwargs):
>  if os.getuid() == 0:
>  case_notrun('{}: cannot be run as root'.format(args[0]))
> +return None
>  else:
>  return func(*args, **kwargs)
>  return func_wrapper

Observation, not demand: this trades one kind of pylint report for
another: inconsistent-return-statements for no-else-return.  PATCH 05
suppresses no-else-return.

[PATCH v9 01/14] iotests: do a light delinting

[John Snow]

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.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 83 ++-
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..886ae962ae 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
 # along with this program.  If not, see .
 #
 
-import errno
 import os
 import re
 import subprocess
-import string
 import unittest
 import sys
 import struct
@@ -35,7 +33,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
 
 faulthandler.enable()
 
@@ -141,11 +139,11 @@ def qemu_img_log(*args):
 return result
 
 def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
-args = [ 'info' ]
+args = ['info']
 if imgopts:
 args.append('--image-opts')
 else:
-args += [ '-f', imgfmt ]
+args += ['-f', imgfmt]
 args += extra_args
 args.append(filename)
 
@@ -224,7 +222,7 @@ def cmd(self, cmd):
 # quit command is in close(), '\n' is added automatically
 assert '\n' not in cmd
 cmd = cmd.strip()
-assert cmd != 'q' and cmd != 'quit'
+assert cmd not in ('q', 'quit')
 self._p.stdin.write(cmd + '\n')
 self._p.stdin.flush()
 return self._read_output()
@@ -246,10 +244,8 @@ def qemu_nbd_early_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-if exitcode == 0:
-return exitcode, ''
-else:
-return exitcode, subp.communicate()[0]
+
+return exitcode, subp.communicate()[0] if exitcode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -313,7 +309,7 @@ def filter_qmp(qmsg, filter_fn):
 items = qmsg.items()
 
 for k, v in items:
-if isinstance(v, list) or isinstance(v, dict):
+if isinstance(v, (dict, list)):
 qmsg[k] = filter_qmp(v, filter_fn)
 else:
 qmsg[k] = filter_fn(k, v)
@@ -324,7 +320,7 @@ def filter_testfiles(msg):
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_testfiles(value)
 return value
@@ -350,7 +346,7 @@ def filter_imgfmt(msg):
 return msg.replace(imgfmt, 'IMGFMT')
 
 def filter_qmp_imgfmt(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_imgfmt(value)
 return value
@@ -361,7 +357,7 @@ def log(msg, filters=[], indent=None):
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
 msg = flt(msg)
-if isinstance(msg, dict) or isinstance(msg, list):
+if isinstance(msg, (dict, list)):
 # Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
 separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
@@ -372,14 +368,14 @@ def log(msg, filters=[], indent=None):
 print(msg)
 
 class Timeout:
-def __init__(self, seconds, errmsg = "Timeout"):
+def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
-def __exit__(self, type, value, traceback):
+def __exit__(self, exc_type, value, traceback):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -388,7 +384,7 @@ def timeout(self, signum, frame):
 def file_pattern(name):
 return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths(object):
+class FilePaths:
 """
 FilePaths is an auto-generated filename that cleans itself up.
 
@@ -535,11 +531,11 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
 
 def resume_drive(self, drive):