[PATCH v6 9/9] iotests: add pylintrc file

John Snow posted 9 patches 5 years, 8 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
[PATCH v6 9/9] iotests: add pylintrc file
Posted by John Snow 5 years, 8 months ago
Repeatable results. run `pylint iotests.py` and you should get a pass.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
 1 file changed, 20 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..feed506f75
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,20 @@
+[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,
+        missing-docstring,
+        line-too-long,
+        too-many-lines,
+        too-few-public-methods,
+        too-many-arguments,
+        too-many-locals,
+        too-many-branches,
+        too-many-public-methods,
\ No newline at end of file
-- 
2.21.1


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by Eric Blake 5 years, 8 months ago
On 2/26/20 6:06 PM, John Snow wrote:
> Repeatable results. run `pylint iotests.py` and you should get a pass.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>   create mode 100644 tests/qemu-iotests/pylintrc

> +        too-many-branches,
> +        too-many-public-methods,
> \ No newline at end of file

Intentional?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by Markus Armbruster 5 years, 8 months ago
John Snow <jsnow@redhat.com> writes:

> Repeatable results. run `pylint iotests.py` and you should get a pass.

Start your sentences with a capital letter, please.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>  1 file changed, 20 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..feed506f75
> --- /dev/null
> +++ b/tests/qemu-iotests/pylintrc
> @@ -0,0 +1,20 @@
> +[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,
> +        missing-docstring,
> +        line-too-long,
> +        too-many-lines,
> +        too-few-public-methods,
> +        too-many-arguments,
> +        too-many-locals,
> +        too-many-branches,
> +        too-many-public-methods,
> \ No newline at end of file

Add the newline, please.

German pejorative for the too-many- and too-few- warnings: "Müsli".
Implies it's for muesli-knitters / granola-crunchers indulging their
orthorexia.

missing-docstring is not advisable for libraries.  Feels okay here.

line-too-long might be worth cleaning up.  How many of them do we have
now?


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by John Snow 5 years, 8 months ago

On 3/4/20 2:22 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
> 
> Start your sentences with a capital letter, please.
> 

The German complains about the capitalization, but not the sentence
fragment.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>>  1 file changed, 20 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..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[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,
>> +        missing-docstring,
>> +        line-too-long,
>> +        too-many-lines,
>> +        too-few-public-methods,
>> +        too-many-arguments,
>> +        too-many-locals,
>> +        too-many-branches,
>> +        too-many-public-methods,
>> \ No newline at end of file
> 
> Add the newline, please.
> 
> German pejorative for the too-many- and too-few- warnings: "Müsli".
> Implies it's for muesli-knitters / granola-crunchers indulging their
> orthorexia.
> 

They are useful at times as they can suggest when you are going a bit
overboard on "organically grown" design. For cleaning an existing
codebase, it's more of a hindrance to the immediate goal of establishing
a baseline.

(*cough* I try to adhere to them in my own freshly written code, and
disable per-line when I've decided to veto the suggestion. Not
appropriate for a codebase like ours. As Max reminds me, it's just tests
-- don't make them too clever or pretty.)

Regardless. It's not appropriate here and now.

> missing-docstring is not advisable for libraries.  Feels okay here.
> 

Ideally we do start using them, but it's out of scope here. Since I did
some cleanup, I wanted to establish the baseline of what I adhered to.

*not* suggest that it's the destination state.

Adding proper docstrings should be done during mypy conversion once the
types are determined, understood, and enforced. Not before then.

> line-too-long might be worth cleaning up.  How many of them do we have
> now?
> 

Five in iotests.py using the default length of 100. 15 if I limit to 80.

If we agree that 100 is okay, I can tackle this in an amendment patch.
If 80 is okay, I'm going to put it off as one coat of paint too many.

(I will try to clean up the 100+ lines for my next version. I am
hesitant to make a deeper cut because I have the feeling it's the type
of series that will incur a lot of nitpicks on indent style.)

--js


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by Markus Armbruster 5 years, 8 months ago
John Snow <jsnow@redhat.com> writes:

> On 3/4/20 2:22 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>> 
>> Start your sentences with a capital letter, please.
>> 
>
> The German complains about the capitalization, but not the sentence
> fragment.

Heh!

>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>>>  1 file changed, 20 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..feed506f75
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/pylintrc
>>> @@ -0,0 +1,20 @@
>>> +[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,
>>> +        missing-docstring,
>>> +        line-too-long,
>>> +        too-many-lines,
>>> +        too-few-public-methods,
>>> +        too-many-arguments,
>>> +        too-many-locals,
>>> +        too-many-branches,
>>> +        too-many-public-methods,
>>> \ No newline at end of file
>> 
>> Add the newline, please.
>> 
>> German pejorative for the too-many- and too-few- warnings: "Müsli".
>> Implies it's for muesli-knitters / granola-crunchers indulging their
>> orthorexia.
>> 
>
> They are useful at times as they can suggest when you are going a bit
> overboard on "organically grown" design. For cleaning an existing
> codebase, it's more of a hindrance to the immediate goal of establishing
> a baseline.

Yes, gentle nudges to reconsider your code organization can be useful.
But when we run pylint with the goal of getting no output, even warnings
are much more than gentle nudges.

> (*cough* I try to adhere to them in my own freshly written code, and
> disable per-line when I've decided to veto the suggestion. Not
> appropriate for a codebase like ours. As Max reminds me, it's just tests
> -- don't make them too clever or pretty.)
>
> Regardless. It's not appropriate here and now.
>
>> missing-docstring is not advisable for libraries.  Feels okay here.
>> 
>
> Ideally we do start using them, but it's out of scope here. Since I did
> some cleanup, I wanted to establish the baseline of what I adhered to.
>
> *not* suggest that it's the destination state.
>
> Adding proper docstrings should be done during mypy conversion once the
> types are determined, understood, and enforced. Not before then.
>
>> line-too-long might be worth cleaning up.  How many of them do we have
>> now?
>> 
>
> Five in iotests.py using the default length of 100. 15 if I limit to 80.
>
> If we agree that 100 is okay, I can tackle this in an amendment patch.
> If 80 is okay, I'm going to put it off as one coat of paint too many.
>
> (I will try to clean up the 100+ lines for my next version. I am
> hesitant to make a deeper cut because I have the feeling it's the type
> of series that will incur a lot of nitpicks on indent style.)

One step at a time.


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago
On 2/27/20 1:06 AM, John Snow wrote:
> Repeatable results. run `pylint iotests.py` and you should get a pass.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>   1 file changed, 20 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..feed506f75
> --- /dev/null
> +++ b/tests/qemu-iotests/pylintrc
> @@ -0,0 +1,20 @@
> +[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,
> +        missing-docstring,
> +        line-too-long,
> +        too-many-lines,
> +        too-few-public-methods,
> +        too-many-arguments,
> +        too-many-locals,
> +        too-many-branches,
> +        too-many-public-methods,
> \ No newline at end of file
> 

Can you run it in one of the CI jobs?


Re: [PATCH v6 9/9] iotests: add pylintrc file
Posted by John Snow 5 years, 8 months ago

On 2/27/20 9:11 AM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:06 AM, John Snow wrote:
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>>   1 file changed, 20 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..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[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,
>> +        missing-docstring,
>> +        line-too-long,
>> +        too-many-lines,
>> +        too-few-public-methods,
>> +        too-many-arguments,
>> +        too-many-locals,
>> +        too-many-branches,
>> +        too-many-public-methods,
>> \ No newline at end of file
>>
> 
> Can you run it in one of the CI jobs?
> 

Tell me where to add it and I will do so.