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

[John Snow <jsnow@redhat.com>]

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('''
>>       }
>>   ''')