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

[John Snow]

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. Since they are externally 
visible objects, having them near other "header" style information makes 
sense to me.

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