Re: [PATCH v9 05/14] iotests: add pylintrc file

[John Snow]

On 3/30/20 10:45 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
> 
>> This allows others to get repeatable results with pylint. If you run
>> `pylint iotests.py`, you should see a 100% pass.
> 
> Nice.
> 
>>
>> Signed-off-by: John Snow <address@hidden>
>> Reviewed-by: Max Reitz <address@hidden>
>> ---
>>  tests/qemu-iotests/pylintrc | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 tests/qemu-iotests/pylintrc
>>
>> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
>> new file mode 100644
>> index 0000000000..8720b6a0de
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,22 @@
>> +[MESSAGES CONTROL]
>> +
>> +# Disable the message, report, category or checker with the given id(s). You
>> +# can either give multiple identifiers separated by comma (,) or put this
>> +# option multiple times (only on the command line, not in the configuration
>> +# file where it should appear only once). You can also use "--disable=all" 
>> to
>> +# disable everything first and then reenable specific checks. For example, 
>> if
>> +# you want to run only the similarities checker, you can use "--disable=all
>> +# --enable=similarities". If you want to run only the classes checker, but 
>> have
>> +# no Warning level messages displayed, use "--disable=all --enable=classes
>> +# --disable=W".
>> +disable=invalid-name,
>> +        no-else-return,
>> +        too-many-lines,
>> +        too-few-public-methods,
>> +        too-many-arguments,
>> +        too-many-locals,
>> +        too-many-branches,
>> +        too-many-public-methods,
> 
> Keep sorted?
> 

OK, (glances at email) if there's a good reason to respin. So far Kevin
asked me to change the word "atom" to "item" which I would consider a
"Maintainer, take mercy on me and just fix this up, would you?" fix, but
let's see how the rest of my email session goes today.

>> +        # These are temporary, and should be removed:
>> +        missing-docstring,
>> +        line-too-long,
> 
> For what it's worth, I also disable these for checking QAPI code, except
> for no-else-return.  My true reason for keeping no-else-return is of
> course that I agree with pylint these elses are ugly.  But I pretend to
> simply go with the flow ;)
> 

You probably also have a few more of the "too-many" warnings disabled,
too. I was working on refactoring the parser recently and found quite a
few more there that were just not worth fixing.

As for the no-else-return ... I think this is direly legitimate:

if cond_a:
    statement_1
    statement_2
    return statement_3
elif cond_b:
    statement_4
    statement_5
    return statement_6
else:
    statement_7
    statement_8
    return statement_9

When you've got at least three choices and neither one is particularly
"canonical", it can be nice to see them chunked out where they are
visually weighted equally.

That said, for this stuff:

if error_cond:
    return -1
else:
    return 0

Should probably be avoided, although ... it's fine.

I think no-else-return is just going to be an intense matter of style
and taste and cannot be left to the linter, so I think I agree with
leaving it on the disabled list.

--js