[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 10/37] qapi/common.py: delint with pylint
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 10/37] qapi/common.py: delint with pylint |
Date: |
Fri, 18 Sep 2020 13:03:53 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> On 9/17/20 10:15 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/common.py | 16 +++++++---------
>>> scripts/qapi/schema.py | 14 +++++++-------
>>> 2 files changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 87d87b95e5..c665e67495 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -14,6 +14,11 @@
>>> import re
>>>
>>> +EATSPACE = '\033EATSPACE.'
>>> +POINTER_SUFFIX = ' *' + EATSPACE
>>> +c_name_trans = str.maketrans('.-', '__')
>>> +
>>> +
>> You rename and move. pylint gripes about the names, but it doesn't
>> actually ask for the move, as far as I can tell. Can you explain why
>> you move?
>>
>
> Preference. I like constants and globals at the top so you can audit
> any code that runs at import time in one place.
I can buy this argument.
> Since they are
> externally visible objects, having them near other "header" style
> information makes sense to me.
This one I find unconvincing. Functions and classes are just as
visible.
Mention the move in the commit message, along with the (convincing part
of the) rationale.
Aside: EATSPACE and c_name_trans are actually local to common.py.
EATSPACE sort of leaks out only via the contract of mcgen(). The
contract could be rephrased in terms of POINTER_SUFFIX. Not sure it's
worthwhile.
>>> # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>>> # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 ->
>>> ENUM_NAME2
>>> # ENUM24_Name -> ENUM24_NAME
>>> @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None):
>>> return camel_to_upper(type_name) + '_' + c_name(const_name,
>>> False).upper()
>>>
>>> -c_name_trans = str.maketrans('.-', '__')
>
> (This one winds up being a constant, so I renamed it in my v2.)
>
>>> -
>>> -
>>> # Map @name to a valid C identifier.
>>> # If @protect, avoid returning certain ticklish identifiers (like
>>> # C keywords) by prepending 'q_'.
>>> @@ -89,10 +91,6 @@ def c_name(name, protect=True):
>>> return name
>>>
>>> -eatspace = '\033EATSPACE.'
>>> -pointer_suffix = ' *' + eatspace
>>> -
>>> -
>>> class Indent:
>>> """
>>> Indent-level management.
>>> @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int:
>>>
>>> # Generate @code with @kwds interpolated.
>>> -# Obey indent_level, and strip eatspace.
>>> +# Obey INDENT level, and strip EATSPACE.
>> Is the change to INDENT intentional?
>>
>
> Kind of, but it's either late (should have been with the indent
> manager patch) or early (Should be with the patch that moves comments
> into docstrings.)
>
> When this comment becomes a docstring, I use `INDENT` to indicate it
> as a proper object. This in and of itself is prescient, as we are not
> using sphinx/apidoc to generate any documentation about the QAPI
> package yet.
>
> (The pending v2 uses `indent` after you pointed out that it was not a
> constant.)
>
>>> def cgen(code, **kwds):
>>> raw = code % kwds
>>> if INDENT:
>>> raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw,
>>> flags=re.MULTILINE)
>>> - return re.sub(re.escape(eatspace) + r' *', '', raw)
>>> + return re.sub(re.escape(EATSPACE) + r' *', '', raw)
>>>
>>> def mcgen(code, **kwds):
>> [...]
>>
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, (continued)
[PATCH 08/37] qapi/common.py: Remove python compatibility workaround, John Snow, 2020/09/15
[PATCH 10/37] qapi/common.py: delint with pylint, John Snow, 2020/09/15
[PATCH 12/37] qapi/common.py: check with pylint, John Snow, 2020/09/15
[PATCH 11/37] qapi/common.py: Replace one-letter 'c' variable, John Snow, 2020/09/15
[PATCH 14/37] qapi/common.py: Move comments into docstrings, John Snow, 2020/09/15