Re: [PATCH 09/37] qapi/common.py: Add indent manager

[John Snow]

On 9/16/20 11:13 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Code style tools really dislike the use of global keywords, because it
> > generally involves re-binding the name at runtime which can have strange
> > effects depending on when and how that global name is referenced in
> > other modules.
> >
> > Make a little indent level manager instead.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/common.py | 50 +++++++++++++++++++++++++++++-------------
> >   scripts/qapi/visit.py  |  7 +++---
> >   2 files changed, 38 insertions(+), 19 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 4fb265a8bf..87d87b95e5 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -93,33 +93,53 @@ def c_name(name, protect=True):
> >   pointer_suffix = ' *' + eatspace
> >   
> >   
> > -def genindent(count):
> > -    ret = ''
> > -    for _ in range(count):
> > -        ret += ' '
> > -    return ret
> > +class Indent:
>
> Since this class name will be used just once...  Indentation or
> IndentationManager?

Indentation is fine, if you'd like. IndentationManager makes me feel 
ashamed for writing this patch, like I am punishing myself with Java.

> > +    """
> > +    Indent-level management.
> >   
> > +    :param initial: Initial value, default 0.
>
> The only caller passes 0.
>
> Let's drop the parameter, and hardcode the default initial value until
> we have an actual use for another initial value.

The parameter is nice because it gives meaning to the output of repr(), 
see below.

> > +    """
> > +    def __init__(self, initial: int = 0) -> None:
> > +        self._level = initial
> >   
> > -indent_level = 0
> > +    def __int__(self) -> int:
> > +        """Return the indent as an integer."""
>
> Isn't "as an integer" obvious?

Just a compulsion to write complete sentences that got lost in the sea 
of many patches. I'll nix this one, because dunder methods do not need 
to be documented.

> Let's replace "the indent" by "the indentation" globally.

They're both cromulent as nouns and one is shorter.

I'll switch in good faith; do you prefer "Indentation" as a noun?

> > +        return self._level
> >   
> > +    def __repr__(self) -> str:
> > +        return "{}({:d})".format(type(self).__name__, self._level)
>
> Is __repr__ needed?

Yes; it's used in the underflow exception , and it is nice when using 
the python shell interactively.

repr(Indent(4)) == "Indent(4)"

> >   
> > -def push_indent(indent_amount=4):
> > -    global indent_level
> > -    indent_level += indent_amount
> > +    def __str__(self) -> str:
> > +        """Return the indent as a string."""
> > +        return ' ' * self._level
> >   
> > +    def __bool__(self) -> bool:
> > +        """True when there is a non-zero indent."""
> > +        return bool(self._level)
>
> This one lets us shorten
>
>      if int(INDENT):
>
> to
>
>      if INDENT:
>
> There's just one instance.  Marginal.  I'm not rejecting it.

Yep, it's trivial in either direction. Still, because it's a custom 
type, I thought I'd be courteous and have it support bool().

> > -def pop_indent(indent_amount=4):
> > -    global indent_level
> > -    indent_level -= indent_amount
> > +    def push(self, amount: int = 4) -> int:
> > +        """Push `amount` spaces onto the indent, default four."""
> > +        self._level += amount
> > +        return self._level
> > +
> > +    def pop(self, amount: int = 4) -> int:
> > +        """Pop `amount` spaces off of the indent, default four."""
> > +        if self._level < amount:
> > +            raise ArithmeticError(
> > +                "Can't pop {:d} spaces from {:s}".format(amount, repr(self)))
> > +        self._level -= amount
> > +        return self._level
>
> The push / pop names never made sense.  The functions don't push onto /
> pop from a stack, they increment / decrement a counter, and so do the
> methods.  Good opportunity to fix the naming.

OK.

I briefly thought about using __isub__ and __iadd__ to support e.g. 
indent += 4, but actually it'd be annoying to have to specify 4 everywhere.

Since you didn't suggest anything, I am going to change it to 'increase' 
and 'decrease'.

> The @amount parameter has never been used.  I don't mind keeping it.
I'll keep it, because I like having the default amount show up in the 
signature instead of as a magic constant in the function body.

> > +
> > +
> > +INDENT = Indent(0)
>
> Uh, does this really qualify as a constant?  It looks quite variable to
> me...

Ever make a mistake? I thought I did once, but I was mistaken.

> >   # Generate @code with @kwds interpolated.
> >   # Obey indent_level, and strip eatspace.
> >   def cgen(code, **kwds):
> >       raw = code % kwds
> > -    if indent_level:
> > -        indent = genindent(indent_level)
> > -        raw, _ = re.subn(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
> > +    if INDENT:
> > +        raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
> >       return re.sub(re.escape(eatspace) + r' *', '', raw)
> >   
> >   
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 31bf46e7f7..66ce3dc52e 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -19,8 +19,7 @@
> >       gen_endif,
> >       gen_if,
> >       mcgen,
> > -    pop_indent,
> > -    push_indent,
> > +    INDENT,
> >   )
> >   from .gen import QAPISchemaModularCVisitor, ifcontext
> >   from .schema import QAPISchemaObjectType
> > @@ -68,7 +67,7 @@ def gen_visit_object_members(name, base, members, variants):
> >       if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> >   ''',
> >                            name=memb.name, c_name=c_name(memb.name))
> > -            push_indent()
> > +            INDENT.push()
> >           ret += mcgen('''
> >       if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
> >           return false;
> > @@ -77,7 +76,7 @@ def gen_visit_object_members(name, base, members, variants):
> >                        c_type=memb.type.c_name(), name=memb.name,
> >                        c_name=c_name(memb.name))
> >           if memb.optional:
> > -            pop_indent()
> > +            INDENT.pop()
> >               ret += mcgen('''
> >       }
> >   ''')