[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/4] qapi: Brush off some (py)lint
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 4/4] qapi: Brush off some (py)lint |
Date: |
Thu, 05 Mar 2020 06:42:10 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
John Snow <address@hidden> writes:
> On 3/4/20 3:01 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>>
>>> On 2/27/20 9:45 AM, Markus Armbruster wrote:
>>>> Signed-off-by: Markus Armbruster <address@hidden>
>>>
>>> I wrote some pylint cleanup for iotests recently, too. Are you targeting
>>> a subset of pylint errors to clean here?
>>>
>>> (Do any files pass 100%?)
>>
>> Surely you're joking, Mr. Snow!
>>
>> I'm chipping away at pylint's gripes. I ran it with the following
>> messages disabled:
>>
>> bad-whitespace,
>> fixme,
>> invalid-name,
>> missing-docstring,
>> too-few-public-methods,
>> too-many-arguments,
>> too-many-branches,
>> too-many-instance-attributes,
>> too-many-lines,
>> too-many-locals,
>> too-many-statements,
>> unused-argument,
>> unused-wildcard-import,
>>
>> These are not all obviously useless. They're just not what I want to
>> focus on right now.
>>
>
> Yes, understood - so my approach is disable what I don't intend to fix,
> commit the pylintrc to prevent backslide, and move on.
>
> I think we have a difference in what a pylintrc means to us (the goal
> vs. the current status.)
>
> I didn't mean "100% without caveats", just "100% in some subset of checks".
>
> (I assume the answer is still no.)
To turn the answer into a yes, I'd have to disable the messages below,
and some of them I'd rather keep.
Tacking
# pylint: disable=...
to existing troublemakers may or may not be worth the ugliness (it needs
to go on the same line, which almost invariably makes it awkwardly long).
>> Remaining:
>>
>> 1 x C0330: Wrong continued indentation (remove 19 spaces).
>>
>> Accident, will fix in v2.
>>
>> 8 x R0201: Method could be a function (no-self-use)
>>
>> Yes, but the override in a sub-class does use self.
>>
>> 2 x W0212: Access to a protected member _body of a client class
>> (protected-access)
>>
>> Needs cleanup, but not now.
>>
>> 6 x W0401: Wildcard import qapi.common (wildcard-import)
>>
>> Not sure I care. I'd prefer not to have more wildcard imports,
>> though.
>>
>> 2 x W0603: Using the global statement (global-statement)
>>
>> Cleanup is non-trivial. Not now.
>>
>> I also ran pycodestyle-3:
>>
>> 1 x E127 continuation line over-indented for visual indent
>>
>> Same as pylint's C0330, will fix in v2.
>>
>> 3 x E261 at least two spaces before inline comment
>>
>> I blame Emacs. Left for another day.
>>
>> 8 x E501 line too long
>>
>> Left for another day.
>>
>> 1 x E713 test for membership should be 'not in'
>>
>> I missed that one, will fix in v2.
>>
>>> Consider checking in a pylintrc file that lets others run the same
>>> subset of pylint tests as you are doing so that we can prevent future
>>> regressions.
>>
>> Working towards it, slowly.
>>
>>> Take a peek at [PATCH v6 0/9] iotests: use python logging​
>>>
>>> Thanks for this series. I had a very similar series sitting waiting to
>>> go out, but this goes further in a few places.
>>
>> Thanks!
>>