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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 9/22/20 6:22 PM, Eduardo Habkost wrote:
>> On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote:
>>> 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 | 51 +++++++++++++++++++++++++++++-------------
>>>   scripts/qapi/visit.py  |  7 +++---
>>>   2 files changed, 38 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index cee63eb95c..e0c5871b10 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -93,33 +93,52 @@ def c_name(name, protect=True):
>>>   pointer_suffix = ' *' + eatspace
>>>     
>>> -def genindent(count):
>>> -    ret = ''
>>> -    for _ in range(count):
>>> -        ret += ' '
>>> -    return ret
>>> +class Indentation:
>>> +    """
>>> +    Indentation level management.
>>>   +    :param initial: Initial number of spaces, default 0.
>>> +    """
>>> +    def __init__(self, initial: int = 0) -> None:
>>> +        self._level = initial
>>>   -indent_level = 0
>>> +    def __int__(self) -> int:
>>> +        return self._level
>>>   +    def __repr__(self) -> str:
>>> +        return "{}({:d})".format(type(self).__name__, self._level)
>>>   -def push_indent(indent_amount=4):
>>> -    global indent_level
>>> -    indent_level += indent_amount
>>> +    def __str__(self) -> str:
>>> +        """Return the current indentation as a string of spaces."""
>>> +        return ' ' * self._level
>>>   +    def __bool__(self) -> bool:
>>> +        """True when there is a non-zero indentation."""
>>> +        return bool(self._level)
>>>   -def pop_indent(indent_amount=4):
>>> -    global indent_level
>>> -    indent_level -= indent_amount
>>> +    def increase(self, amount: int = 4) -> int:
>>> +        """Increase the indentation level by `amount`, default 4."""
>>> +        self._level += amount
>>> +        return self._level
>>> +
>>> +    def decrease(self, amount: int = 4) -> int:
>>> +        """Decrease the indentation level by `amount`, default 4."""
>>> +        if self._level < amount:
>>> +            raise ArithmeticError(
>>> +                f"Can't remove {amount:d} spaces from {self!r}")
>>> +        self._level -= amount
>>> +        return self._level
>>> +
>>> +
>>> +indent = Indentation()
>> Personally, I would keep the push_indent(), pop_indent() API, and
>> introduce an indent() function, to follow the existing API style
>> of plain functions.
>> Something like:
>>    indent_prefixes = []
>>    def push_indent(amount=4):
>>        """Add `amount` spaces to indentation prefix"""
>>        indent_prefixes.push(' '*amount)
>>    def pop_indent():
>>        """Revert the most recent push_indent() call"""
>>        indent_prefixes.pop()
>>    def indent():
>>        """Return the current indentation prefix"""
>>        return ''.join(indent_prefixes)
>> What you did is still an improvement, though, so:
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> 
>
> Yeah, there's only one user right now, so ... I just kinda wanted to
> get rid of the global usage. Maybe if we make the code generator
> fancier we'll find out what form is best.

You don't get rid of the global variable, you just change it from
integer to a class.  A class can be handier when generating multiple
things interleaved, because you can have one class instance per thing.

Note that we already have a class instance per thing we generate:
instances of subtypes of QAPIGen.  The thought of moving the indentation
machinery into QAPIGen or or maybe QAPIGenCCode crossed my mind many
moons ago, but I had bigger fish to fry, and then I forgot :)

John, I suggest you don't try to make this pretty just yet.  Do what
needs to be done for the type hint job.  We can make it pretty later.