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

[John Snow <jsnow@redhat.com>]

On 9/17/20 4:07 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 9/16/20 11:13 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>


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

I made the change; see gitlab commits or wait for v2 to see if it reads 
better to you.

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

We'll get to them in due time. For now, please admire the lipstick.


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

Secretly, asserts can be disabled in Python just like they can in C code.

My habit: if it's something that should already be true given the nature 
of how the code is laid out, use an assertion. If I am preventing an 
erroneous state (Especially from callers in other modules), explicitly 
raise an exception.

(See the gitlab branch for the updated version of this patch, which has 
changed the code slightly.)

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

increase and decrease it is.

>>> 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!"
> 

:)