[PATCH 0/2] pylint: fix new errors and warnings

[PATCH 0/2] pylint: fix new errors and warnings

[Emanuele Giuseppe Esposito]

There are some warnings and errors that we either miss or
are new in pylint. Anyways, test 297 of qemu-iotests fails
because of that, so we need to fix it.

All these fixes involve just indentation or additional spaces
added.

Emanuele Giuseppe Esposito (2):
  pylint: fix errors and warnings from qemu-tests test 297
  qemu-iotests: fix image-fleecing pylint errors

 tests/qemu-iotests/129                  |  9 ++---
 tests/qemu-iotests/310                  | 16 ++++-----
 tests/qemu-iotests/check                | 11 +++---
 tests/qemu-iotests/iotests.py           |  7 ++--
 tests/qemu-iotests/tests/image-fleecing | 47 +++++++++++++++----------
 5 files changed, 51 insertions(+), 39 deletions(-)

-- 
2.27.0

[PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Emanuele Giuseppe Esposito]

Test 297 in qemu-iotests folder currently fails: pylint has
learned new things to check, or we simply missed them.

All fixes in this patch are related to additional spaces used
or wrong indentation.

No functional change intended.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/129                  |  9 +++++----
 tests/qemu-iotests/310                  | 16 ++++++++--------
 tests/qemu-iotests/check                | 11 ++++++-----
 tests/qemu-iotests/iotests.py           |  7 ++++---
 tests/qemu-iotests/tests/image-fleecing |  4 ++--
 5 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 5251e2669e..21df239597 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -77,8 +77,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         self.do_test_stop("drive-backup", device="drive0",
                           target=self.target_img, format=iotests.imgfmt,
                           sync="full",
-                          x_perf={ 'max-chunk': 65536,
-                                   'max-workers': 8 })
+                          x_perf={'max-chunk': 65536,
+                                  'max-workers': 8})
 
     def test_block_commit(self):
         # Add overlay above the source node so that we actually use a
@@ -87,13 +87,14 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
                          '1G')
 
-        result = self.vm.qmp('blockdev-add', **{
+        result = self.vm.qmp('blockdev-add',
+                             **{
                                  'node-name': 'overlay',
                                  'driver': iotests.imgfmt,
                                  'file': {
                                      'driver': 'file',
                                      'filename': self.overlay_img
-                                 }
+                                     }
                              })
         self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
index 9d9c928c4b..33c3411869 100755
--- a/tests/qemu-iotests/310
+++ b/tests/qemu-iotests/310
@@ -48,11 +48,11 @@ with iotests.FilePath('base.img') as base_img_path, \
     assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
     assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
                     '-F', iotests.imgfmt, mid_img_path) == 0
-    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 2M 1M') == 0
-    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 4M 1M') == 0
+    assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 2M 1M') == 0
+    assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 4M 1M') == 0
     assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
                     '-F', iotests.imgfmt, top_img_path) == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'write -P 2 1M 1M') == 0
 
 #      0 1 2 3 4
 # top    2
@@ -108,10 +108,10 @@ with iotests.FilePath('base.img') as base_img_path, \
     assert qemu_img('rebase', '-u', '-b', '', '-f', iotests.imgfmt,
                     top_img_path) == 0
 
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 0 1M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 2M 1M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 3M 1M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 4M 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'read -P 0 0 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'read -P 2 1M 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'read -P 3 2M 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'read -P 0 3M 1M') == 0
+    assert qemu_io_silent(top_img_path, '-c', 'read -P 3 4M 1M') == 0
 
     log('Done')
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index da1bfb839e..43a4b694cc 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -37,13 +37,14 @@ def make_argparser() -> argparse.ArgumentParser:
 
     p.add_argument('-d', dest='debug', action='store_true', help='debug')
     p.add_argument('-p', dest='print', action='store_true',
-                help='redirects qemu\'s stdout and stderr to the test output')
+                   help='redirects qemu\'s stdout and stderr to '
+                        'the test output')
     p.add_argument('-gdb', action='store_true',
-                   help="start gdbserver with $GDB_OPTIONS options \
-                        ('localhost:12345' if $GDB_OPTIONS is empty)")
+                   help="start gdbserver with $GDB_OPTIONS options "
+                        "('localhost:12345' if $GDB_OPTIONS is empty)")
     p.add_argument('-valgrind', action='store_true',
-                    help='use valgrind, sets VALGRIND_QEMU environment '
-                    'variable')
+                   help='use valgrind, sets VALGRIND_QEMU environment '
+                        'variable')
 
     p.add_argument('-misalign', action='store_true',
                    help='misalign memory allocations')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf5630..168cc5736a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -608,7 +608,7 @@ def _post_shutdown(self) -> None:
         super()._post_shutdown()
         if not qemu_valgrind or not self._popen:
             return
-        valgrind_filename =  f"{test_dir}/{self._popen.pid}.valgrind"
+        valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"
         if self.exitcode() == 99:
             with open(valgrind_filename, encoding='utf-8') as f:
                 print(f.read())
@@ -1350,8 +1350,9 @@ def write(self, arg=None):
 
 class ReproducibleTestRunner(unittest.TextTestRunner):
     def __init__(self, stream: Optional[TextIO] = None,
-             resultclass: Type[unittest.TestResult] = ReproducibleTestResult,
-             **kwargs: Any) -> None:
+                 resultclass: Type[unittest.TestResult] =
+                 ReproducibleTestResult,
+                 **kwargs: Any) -> None:
         rstream = ReproducibleStreamWrapper(stream or sys.stdout)
         super().__init__(stream=rstream,           # type: ignore
                          descriptions=True,
diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index f6318492c6..8c5472f421 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -114,8 +114,8 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
 
     nbd_uri = 'nbd+unix:///%s?socket=%s' % (tmp_node, nbd_sock_path)
     log(vm.qmp('nbd-server-start',
-               {'addr': { 'type': 'unix',
-                          'data': { 'path': nbd_sock_path } } }))
+               {'addr': {'type': 'unix',
+                         'data': {'path': nbd_sock_path}}}))
 
     log(vm.qmp('nbd-server-add', device=tmp_node))
 
-- 
2.27.0

[PATCH 2/2] qemu-iotests: fix image-fleecing pylint errors

[Emanuele Giuseppe Esposito]

The problem here is that some variables are formatted with
unnecessary spaces to make it prettier and easier to read.

However, pylint complains about those additional spaces.
A solution is to transform them as string with arbitrary spaces,
and then convert it back into a tuple.

Removing the spaces makes it a little bit ugly, and directly
using the string forces us to change the test reference output
accordingly, which will 1) contain ugly weird formatted strings,
2) is not portable if somebody changes the formatting in the test
string.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/tests/image-fleecing | 43 +++++++++++++++----------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index 8c5472f421..9709ecbba1 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -30,23 +30,27 @@ iotests.script_initialize(
     supported_platforms=['linux'],
 )
 
-patterns = [('0x5d', '0',         '64k'),
-            ('0xd5', '1M',        '64k'),
-            ('0xdc', '32M',       '64k'),
-            ('0xcd', '0x3ff0000', '64k')]  # 64M - 64K
-
-overwrite = [('0xab', '0',         '64k'), # Full overwrite
-             ('0xad', '0x00f8000', '64k'), # Partial-left (1M-32K)
-             ('0x1d', '0x2008000', '64k'), # Partial-right (32M+32K)
-             ('0xea', '0x3fe0000', '64k')] # Adjacent-left (64M - 128K)
-
-zeroes = [('0', '0x00f8000', '32k'), # Left-end of partial-left (1M-32K)
-          ('0', '0x2010000', '32k'), # Right-end of partial-right (32M+64K)
-          ('0', '0x3fe0000', '64k')] # overwrite[3]
-
-remainder = [('0xd5', '0x108000',  '32k'), # Right-end of partial-left [1]
-             ('0xdc', '32M',       '32k'), # Left-end of partial-right [2]
-             ('0xcd', '0x3ff0000', '64k')] # patterns[3]
+# Each string in patterns, overwrite and remainder is formatted in such way
+# for readability. Using a 3-tuple will make pylint trigger
+# "bad-whitespace" when we try to format it in the same way
+# The testing code will take care of splitting it properly.
+patterns = ['0x5d 0         64k',
+            '0xd5 1M        64k',
+            '0xdc 32M       64k',
+            '0xcd 0x3ff0000 64k']  # 64M - 64K
+
+overwrite = ['0xab 0         64k', # Full overwrite
+             '0xad 0x00f8000 64k', # Partial-left (1M-32K)
+             '0x1d 0x2008000 64k', # Partial-right (32M+32K)
+             '0xea 0x3fe0000 64k'] # Adjacent-left (64M - 128K)
+
+zeroes = ['0 0x00f8000 32k', # Left-end of partial-left (1M-32K)
+          '0 0x2010000 32k', # Right-end of partial-right (32M+64K)
+          '0 0x3fe0000 64k'] # overwrite[3]
+
+remainder = ['0xd5 0x108000  32k', # Right-end of partial-left [1]
+             '0xdc 32M       32k', # Left-end of partial-right [2]
+             '0xcd 0x3ff0000 64k'] # patterns[3]
 
 def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     log('--- Setting up images ---')
@@ -56,6 +60,7 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     assert qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M') == 0
 
     for p in patterns:
+        p = tuple(p.split())
         qemu_io('-f', iotests.imgfmt,
                 '-c', 'write -P%s %s %s' % p, base_img_path)
 
@@ -124,6 +129,7 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     log('')
 
     for p in patterns + zeroes:
+        p = tuple(p.split())
         cmd = 'read -P%s %s %s' % p
         log(cmd)
         assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
@@ -133,6 +139,7 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     log('')
 
     for p in overwrite:
+        p = tuple(p.split())
         cmd = 'write -P%s %s %s' % p
         log(cmd)
         log(vm.hmp_qemu_io(qom_path, cmd, qdev=True))
@@ -142,6 +149,7 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     log('')
 
     for p in patterns + zeroes:
+        p = tuple(p.split())
         cmd = 'read -P%s %s %s' % p
         log(cmd)
         assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
@@ -168,6 +176,7 @@ def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
     log('')
 
     for p in overwrite + remainder:
+        p = tuple(p.split())
         cmd = 'read -P%s %s %s' % p
         log(cmd)
         assert qemu_io_silent(base_img_path, '-c', cmd) == 0
-- 
2.27.0

Re: [PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Kevin Wolf]

Am 06.10.2021 um 15:00 hat Emanuele Giuseppe Esposito geschrieben:
> Test 297 in qemu-iotests folder currently fails: pylint has
> learned new things to check, or we simply missed them.
> 
> All fixes in this patch are related to additional spaces used
> or wrong indentation.
> 
> No functional change intended.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

> @@ -87,13 +87,14 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>          iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
>                           '1G')
>  
> -        result = self.vm.qmp('blockdev-add', **{
> +        result = self.vm.qmp('blockdev-add',
> +                             **{
>                                   'node-name': 'overlay',
>                                   'driver': iotests.imgfmt,
>                                   'file': {
>                                       'driver': 'file',
>                                       'filename': self.overlay_img
> -                                 }
> +                                     }
>                               })
>          self.assert_qmp(result, 'return', {})

Am I the only one to think that the new indentation for the closing
brace there is horrible? PEP-8 explictly allows things like:

    my_list = [
        1, 2, 3,
        4, 5, 6,
    ]

Some of the other changes in this patch should be made, but at least if
these are behind different switches, I would consider just disabling the
one that complains about nicely formatted dicts.

Kevin

Re: [PATCH 2/2] qemu-iotests: fix image-fleecing pylint errors

[Kevin Wolf]

Am 06.10.2021 um 15:01 hat Emanuele Giuseppe Esposito geschrieben:
> The problem here is that some variables are formatted with
> unnecessary spaces to make it prettier and easier to read.
> 
> However, pylint complains about those additional spaces.
> A solution is to transform them as string with arbitrary spaces,
> and then convert it back into a tuple.
> 
> Removing the spaces makes it a little bit ugly, and directly
> using the string forces us to change the test reference output
> accordingly, which will 1) contain ugly weird formatted strings,
> 2) is not portable if somebody changes the formatting in the test
> string.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Changing our logic because of a style checker feels wrong. I'd rather
stick in a line like this before the definitions:

# pylint: disable=bad-whitespace

(Not sure if the syntax of this is entirely correct, but from the
comment in your patch and existing uses in iotests, I think this would
be the line.)

Kevin

Re: [PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Emanuele Giuseppe Esposito]

On 06/10/2021 18:46, Kevin Wolf wrote:
> Am 06.10.2021 um 15:00 hat Emanuele Giuseppe Esposito geschrieben:
>> Test 297 in qemu-iotests folder currently fails: pylint has
>> learned new things to check, or we simply missed them.
>>
>> All fixes in this patch are related to additional spaces used
>> or wrong indentation.
>>
>> No functional change intended.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
>> @@ -87,13 +87,14 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
>>                            '1G')
>>   
>> -        result = self.vm.qmp('blockdev-add', **{
>> +        result = self.vm.qmp('blockdev-add',
>> +                             **{
>>                                    'node-name': 'overlay',
>>                                    'driver': iotests.imgfmt,
>>                                    'file': {
>>                                        'driver': 'file',
>>                                        'filename': self.overlay_img
>> -                                 }
>> +                                     }
>>                                })
>>           self.assert_qmp(result, 'return', {})
> 
> Am I the only one to think that the new indentation for the closing
> brace there is horrible? PEP-8 explictly allows things like:
> 
>      my_list = [
>          1, 2, 3,
>          4, 5, 6,
>      ]
> 
> Some of the other changes in this patch should be made, but at least if
> these are behind different switches, I would consider just disabling the
> one that complains about nicely formatted dicts.

The error is "C0330: Wrong hanging indentation"
so it is not about dicts. I guess we can disable the error, but the 
problem is that we will disable it for the whole file, which doesn't 
seem right.

Alternatively, this also works fine:

-        result = self.vm.qmp('blockdev-add',
-                             **{
-                                 'node-name': 'overlay',
-                                 'driver': iotests.imgfmt,
-                                 'file': {
-                                     'driver': 'file',
-                                     'filename': self.overlay_img
-                                     }
-                             })
+        result = self.vm.qmp('blockdev-add', **{
+            'node-name': 'overlay',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': self.overlay_img
+            }})

What do you think?

Otherwise I am happy to disable the error altogether.

Emanuele

Re: [PATCH 2/2] qemu-iotests: fix image-fleecing pylint errors

[Emanuele Giuseppe Esposito]

On 06/10/2021 18:51, Kevin Wolf wrote:
> Am 06.10.2021 um 15:01 hat Emanuele Giuseppe Esposito geschrieben:
>> The problem here is that some variables are formatted with
>> unnecessary spaces to make it prettier and easier to read.
>>
>> However, pylint complains about those additional spaces.
>> A solution is to transform them as string with arbitrary spaces,
>> and then convert it back into a tuple.
>>
>> Removing the spaces makes it a little bit ugly, and directly
>> using the string forces us to change the test reference output
>> accordingly, which will 1) contain ugly weird formatted strings,
>> 2) is not portable if somebody changes the formatting in the test
>> string.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Changing our logic because of a style checker feels wrong. I'd rather
> stick in a line like this before the definitions:
> 
> # pylint: disable=bad-whitespace
> 
> (Not sure if the syntax of this is entirely correct, but from the
> comment in your patch and existing uses in iotests, I think this would
> be the line.)

Ok, I will add the line. Same remarks from the previous patch applies: 
unfortunately then we disable the warning for the whole file.

Since here (like the previous patch) the error spans on multiple lines, 
adding a # pylint: disable= comment on each line is infeasible and ugly.

Emanuele

Re: [PATCH 2/2] qemu-iotests: fix image-fleecing pylint errors

[Kevin Wolf]

Am 07.10.2021 um 09:53 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> On 06/10/2021 18:51, Kevin Wolf wrote:
> > Am 06.10.2021 um 15:01 hat Emanuele Giuseppe Esposito geschrieben:
> > > The problem here is that some variables are formatted with
> > > unnecessary spaces to make it prettier and easier to read.
> > > 
> > > However, pylint complains about those additional spaces.
> > > A solution is to transform them as string with arbitrary spaces,
> > > and then convert it back into a tuple.
> > > 
> > > Removing the spaces makes it a little bit ugly, and directly
> > > using the string forces us to change the test reference output
> > > accordingly, which will 1) contain ugly weird formatted strings,
> > > 2) is not portable if somebody changes the formatting in the test
> > > string.
> > > 
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > Changing our logic because of a style checker feels wrong. I'd rather
> > stick in a line like this before the definitions:
> > 
> > # pylint: disable=bad-whitespace
> > 
> > (Not sure if the syntax of this is entirely correct, but from the
> > comment in your patch and existing uses in iotests, I think this would
> > be the line.)
> 
> Ok, I will add the line. Same remarks from the previous patch applies:
> unfortunately then we disable the warning for the whole file.
> 
> Since here (like the previous patch) the error spans on multiple lines,
> adding a # pylint: disable= comment on each line is infeasible and ugly.

It doesn't fail with my pylint version, so I can't try it out, but does
the following work?

# pylint: disable=bad-whitespace
... definitions ...
# pylint: enable=bad-whitespace

Kevin

Re: [PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Kevin Wolf]

Am 07.10.2021 um 09:51 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> On 06/10/2021 18:46, Kevin Wolf wrote:
> > Am 06.10.2021 um 15:00 hat Emanuele Giuseppe Esposito geschrieben:
> > > Test 297 in qemu-iotests folder currently fails: pylint has
> > > learned new things to check, or we simply missed them.
> > > 
> > > All fixes in this patch are related to additional spaces used
> > > or wrong indentation.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > > @@ -87,13 +87,14 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
> > >           iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
> > >                            '1G')
> > > -        result = self.vm.qmp('blockdev-add', **{
> > > +        result = self.vm.qmp('blockdev-add',
> > > +                             **{
> > >                                    'node-name': 'overlay',
> > >                                    'driver': iotests.imgfmt,
> > >                                    'file': {
> > >                                        'driver': 'file',
> > >                                        'filename': self.overlay_img
> > > -                                 }
> > > +                                     }
> > >                                })
> > >           self.assert_qmp(result, 'return', {})
> > 
> > Am I the only one to think that the new indentation for the closing
> > brace there is horrible? PEP-8 explictly allows things like:
> > 
> >      my_list = [
> >          1, 2, 3,
> >          4, 5, 6,
> >      ]
> > 
> > Some of the other changes in this patch should be made, but at least if
> > these are behind different switches, I would consider just disabling the
> > one that complains about nicely formatted dicts.
> 
> The error is "C0330: Wrong hanging indentation"
> so it is not about dicts. I guess we can disable the error, but the problem
> is that we will disable it for the whole file, which doesn't seem right.

Actually, I would disable it globally in pylintrc because building
dictionaries for JSON is something that we do a lot.

But then I'm surprised that this is the only instance that actually
fails. I wonder what the difference is.

For example, 129 doesn't seem to be skipped and has this code:

    result = self.vm.qmp('blockdev-add', **{
                             'node-name': 'overlay',
                             'driver': iotests.imgfmt,
                             'file': {
                                 'driver': 'file',
                                 'filename': self.overlay_img
                             }
                         })

Yet you don't report a pylint error for this file.

Oooh... I think I do see a difference: The final line is indented by one
space more in the case that fails for you. It should be vertically
aligned with the "'" in the first line, but it is actually aligned with
the "b" of "blockdev-add".

Does removing one space of indentation in the last line fix the report?

Kevin

Re: [PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Emanuele Giuseppe Esposito]

>> The error is "C0330: Wrong hanging indentation"
>> so it is not about dicts. I guess we can disable the error, but the problem
>> is that we will disable it for the whole file, which doesn't seem right.
> 
> Actually, I would disable it globally in pylintrc because building
> dictionaries for JSON is something that we do a lot.
> 
> But then I'm surprised that this is the only instance that actually
> fails. I wonder what the difference is.
> 
> For example, 129 doesn't seem to be skipped and has this code:


> 
>      result = self.vm.qmp('blockdev-add', **{
>                               'node-name': 'overlay',
>                               'driver': iotests.imgfmt,
>                               'file': {
>                                   'driver': 'file',
>                                   'filename': self.overlay_img
>                               }
>                           })
> 
> Yet you don't report a pylint error for this file.

Well, unless I am misunderstanding something... 129 *is* the file I am 
reporting. And that is exactly the function where pylint complains.

> 
> Oooh... I think I do see a difference: The final line is indented by one
> space more in the case that fails for you. It should be vertically
> aligned with the "'" in the first line, but it is actually aligned with
> the "b" of "blockdev-add"
> 
> Does removing one space of indentation in the last line fix the report?

It's not only the final line, it's from "**{" till the ending ")".
'node-name' is under "ock" of 'blockdev-add'. It is clearly bad 
indented, regardless of the new style and pylint new rules.

Pylint itself suggests to move it 4 spaces more than "result =", ie 21 
spaces before.

Still, applying your suggestion to all the lines and removing 1 space 
from all lines still does not make pylint happy, as it asks to remove 20 
spaces.

To simplify things, this is the error I get:

  === pylint ===
+************* Module 129
+129:91:0: C0330: Wrong hanging indentation (remove 21 spaces).
+                                 'node-name': 'overlay',
+            |                    ^ (bad-continuation)
+129:92:0: C0330: Wrong hanging indentation (remove 21 spaces).
+                                 'driver': iotests.imgfmt,
+            |                    ^ (bad-continuation)
+129:93:0: C0330: Wrong hanging indentation (remove 21 spaces).
+                                 'file': {
+            |                    ^ (bad-continuation)
+129:97:0: C0330: Wrong hanging indentation.
+                             })
+        |   |                ^ (bad-continuation)

So unless you want to disable it overall, one way of fixing 129 is to 
follow what pylint suggests, and do like I wrote in the previous email:

Either:
         result = self.vm.qmp('blockdev-add', **{
             'node-name': 'overlay', 		<-- 21 spaces less
             'driver': iotests.imgfmt,		<-- 21 spaces less
             'file': {				<-- 21 spaces less
                 'driver': 'file',		<-- 21 spaces less
                 'filename': self.overlay_img	<-- 21 spaces less
             }					<-- 21 spaces less
         })					<-- 21 spaces less
	
or (difference is in the last line only):
         result = self.vm.qmp('blockdev-add', **{
             'node-name': 'overlay', 		<-- 21 spaces less
             'driver': iotests.imgfmt,		<-- 21 spaces less
             'file': {				<-- 21 spaces less
                 'driver': 'file',		<-- 21 spaces less
                 'filename': self.overlay_img	<-- 21 spaces less
             }})					<-- 21 spaces less
Emanuele

Re: [PATCH 2/2] qemu-iotests: fix image-fleecing pylint errors

[Emanuele Giuseppe Esposito]

On 07/10/2021 10:36, Kevin Wolf wrote:
> Am 07.10.2021 um 09:53 hat Emanuele Giuseppe Esposito geschrieben:
>>
>>
>> On 06/10/2021 18:51, Kevin Wolf wrote:
>>> Am 06.10.2021 um 15:01 hat Emanuele Giuseppe Esposito geschrieben:
>>>> The problem here is that some variables are formatted with
>>>> unnecessary spaces to make it prettier and easier to read.
>>>>
>>>> However, pylint complains about those additional spaces.
>>>> A solution is to transform them as string with arbitrary spaces,
>>>> and then convert it back into a tuple.
>>>>
>>>> Removing the spaces makes it a little bit ugly, and directly
>>>> using the string forces us to change the test reference output
>>>> accordingly, which will 1) contain ugly weird formatted strings,
>>>> 2) is not portable if somebody changes the formatting in the test
>>>> string.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>
>>> Changing our logic because of a style checker feels wrong. I'd rather
>>> stick in a line like this before the definitions:
>>>
>>> # pylint: disable=bad-whitespace
>>>
>>> (Not sure if the syntax of this is entirely correct, but from the
>>> comment in your patch and existing uses in iotests, I think this would
>>> be the line.)
>>
>> Ok, I will add the line. Same remarks from the previous patch applies:
>> unfortunately then we disable the warning for the whole file.
>>
>> Since here (like the previous patch) the error spans on multiple lines,
>> adding a # pylint: disable= comment on each line is infeasible and ugly.
> 
> It doesn't fail with my pylint version, so I can't try it out, but does
> the following work?
> 
> # pylint: disable=bad-whitespace
> ... definitions ...
> # pylint: enable=bad-whitespace

You are right, this is valid and looks very good. In this way I can just 
temporarily disable the error for the variables.

> 
> Kevin
> 

Re: [PATCH 1/2] pylint: fix errors and warnings from qemu-tests test 297

[Kevin Wolf]

Am 07.10.2021 um 12:34 hat Emanuele Giuseppe Esposito geschrieben:
> 
> > > The error is "C0330: Wrong hanging indentation"
> > > so it is not about dicts. I guess we can disable the error, but the problem
> > > is that we will disable it for the whole file, which doesn't seem right.
> > 
> > Actually, I would disable it globally in pylintrc because building
> > dictionaries for JSON is something that we do a lot.
> > 
> > But then I'm surprised that this is the only instance that actually
> > fails. I wonder what the difference is.
> > 
> > For example, 129 doesn't seem to be skipped and has this code:
> 
> 
> > 
> >      result = self.vm.qmp('blockdev-add', **{
> >                               'node-name': 'overlay',
> >                               'driver': iotests.imgfmt,
> >                               'file': {
> >                                   'driver': 'file',
> >                                   'filename': self.overlay_img
> >                               }
> >                           })
> > 
> > Yet you don't report a pylint error for this file.
> 
> Well, unless I am misunderstanding something... 129 *is* the file I am
> reporting. And that is exactly the function where pylint complains.

Indeed, my bad. I got confused there.

And the other files that do something similar are all in SKIP_FILES in
297. So it looks like we don't have another case to copy.

> > 
> > Oooh... I think I do see a difference: The final line is indented by one
> > space more in the case that fails for you. It should be vertically
> > aligned with the "'" in the first line, but it is actually aligned with
> > the "b" of "blockdev-add"
> > 
> > Does removing one space of indentation in the last line fix the report?
> 
> It's not only the final line, it's from "**{" till the ending ")".
> 'node-name' is under "ock" of 'blockdev-add'. It is clearly bad indented,
> regardless of the new style and pylint new rules.
> 
> Pylint itself suggests to move it 4 spaces more than "result =", ie 21
> spaces before.
> 
> Still, applying your suggestion to all the lines and removing 1 space from
> all lines still does not make pylint happy, as it asks to remove 20 spaces.
> 
> To simplify things, this is the error I get:
> 
>  === pylint ===
> +************* Module 129
> +129:91:0: C0330: Wrong hanging indentation (remove 21 spaces).
> +                                 'node-name': 'overlay',
> +            |                    ^ (bad-continuation)
> +129:92:0: C0330: Wrong hanging indentation (remove 21 spaces).
> +                                 'driver': iotests.imgfmt,
> +            |                    ^ (bad-continuation)
> +129:93:0: C0330: Wrong hanging indentation (remove 21 spaces).
> +                                 'file': {
> +            |                    ^ (bad-continuation)
> +129:97:0: C0330: Wrong hanging indentation.
> +                             })
> +        |   |                ^ (bad-continuation)
> 
> So unless you want to disable it overall, one way of fixing 129 is to follow
> what pylint suggests, and do like I wrote in the previous email:
> 
> Either:
>         result = self.vm.qmp('blockdev-add', **{
>             'node-name': 'overlay', 		<-- 21 spaces less
>             'driver': iotests.imgfmt,		<-- 21 spaces less
>             'file': {				<-- 21 spaces less
>                 'driver': 'file',		<-- 21 spaces less
>                 'filename': self.overlay_img	<-- 21 spaces less
>             }					<-- 21 spaces less
>         })					<-- 21 spaces less

Yes, this looks reasonble enough.

Kevin