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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

YAGNI.

> I prefer what I've currently written.

Where personal preference collides with local consistency, I'm with
local consistency.

You can get the two in line: change everything to your preference.

You signalled intent to do that for __repr__(): "Give me time".
Alright, having such __repr__() is obviously more important / useful to
you than avoiding the extra code is to me.

I received no such signal for checking preconditions.  Good, because I'd
go "are you serious?" :)