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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

If I take off my glasses and step six feet back, I just might be able to
overlook it.

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

There are compilers that let you switch off array bounds checking.
Would you advocate manual bounds checking to protect people from their
own folly?

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

I check function preconditions ruthlessly with assert.  There's no sane
way to recover anyway.

Without a way to recover, the only benefit is crashing more prettily.
If the error is six levels deep in a some fancy-pants framework, then
prettier crashes might actually help someone finding the error slightly
faster.  But it ain't.

My final argument is local consistency: use of assertions to check
preconditions is pervasive in scripts/qapi/.

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

[...]