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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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.

Hah!  Point taken.

>>> +    """
>>> +    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?

Use of "indent" as a noun was new to me, but what do I know; you're the
native speaker.  Use your judgement.  Applies to the class name, too.

>>> +        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)"

Meh.  There's another three dozen classes for you to put lipstick on :)

>>>   -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)))

I think assert(amount <= self._level) would do just fine.

>>> +        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'.

Works for me.  So would inc and dec.

>> 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.

"If I had any humility, I'd be perfect!"