Re: [PATCH 10/37] qapi/common.py: delint with pylint

[Markus Armbruster <armbru@redhat.com>]

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):
>> [...]
>>