[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 09/37] qapi/common.py: Add indent manager
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 09/37] qapi/common.py: Add indent manager |
Date: |
Mon, 21 Sep 2020 09:43:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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?" :)
- Re: [PATCH 07/37] qapi: add pylintrc, (continued)
[PATCH 09/37] qapi/common.py: Add indent manager, John Snow, 2020/09/15
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, Markus Armbruster, 2020/09/16
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, John Snow, 2020/09/16
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, Markus Armbruster, 2020/09/17
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, John Snow, 2020/09/17
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, Markus Armbruster, 2020/09/18
- Re: [PATCH 09/37] qapi/common.py: Add indent manager, John Snow, 2020/09/18
- Re: [PATCH 09/37] qapi/common.py: Add indent manager,
Markus Armbruster <=
[PATCH 08/37] qapi/common.py: Remove python compatibility workaround, John Snow, 2020/09/15
[PATCH 10/37] qapi/common.py: delint with pylint, John Snow, 2020/09/15
[PATCH 12/37] qapi/common.py: check with pylint, John Snow, 2020/09/15
[PATCH 11/37] qapi/common.py: Replace one-letter 'c' variable, John Snow, 2020/09/15