Am 03.03.2020 um 22:25 hat John Snow geschrieben: > > > 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.
I'm not sure I understand this connection. mypy doesn't care about style. > >> 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. Max can't be the wrongest person for it because that's already me. > >> 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. Here and... > >> @@ -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)) ...here I must say that while I think Max's suggestions feel like an improvement to me over the patch, I actually like the current code best in both cases. In fact, after scanning your patch, I feel it's actually the majority of changes that pylint wants that aren't an improvement... Maybe this just underlines the fact that I am the wrongest person to review such patches and not Max. Though I'm surprised that I'm generally not the person who introduces the code violating the rules, and I don't have the impression in this thread that anyone is eager to defend pylint's opinion. Now I ran pylint myself and it prints some even more ridiculous warnings like variable names being too short for its liking. I guess this means that if we want to run it without warnings or errors, we need to use a config file anyway to disable the worst parts. And if we have a config file anyway, maybe we can more selectively enable the checks that we actually want? Kevin
