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

[John Snow <jsnow@redhat.com>]

On 9/18/20 6:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:

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

I consider writing a nice __repr__ good habit, I'd prefer not to delete 
it just because the rest of our code doesn't do so yet. (Give me time.)

I spend a lot of my time in the interactive python console: having nice 
__repr__ methods is a good habit, not an unsightly blemish.

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

You're right that there's no safe recovery from an error such as this. 
The only actual difference is whether you see AssertionError or 
ArithmeticError.

One can be disabled (But you rightly shouldn't), the other can't. One 
has more semantic meaning and information to it.

I prefer what I've currently written.

--js