| Message ID | 20211006130100.389521-2-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pylint: fix new errors and warnings | expand |
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
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
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
>> 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
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
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))
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(-)