John Snow <js...@redhat.com> writes:
> On 9/16/20 11:13 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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!"
