diff mbox series

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

Message ID 20200915224027.2529813-10-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 15, 2020, 10:39 p.m. UTC
Code style tools really dislike the use of global keywords, because it
generally involves re-binding the name at runtime which can have strange
effects depending on when and how that global name is referenced in
other modules.

Make a little indent level manager instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/common.py | 50 +++++++++++++++++++++++++++++-------------
 scripts/qapi/visit.py  |  7 +++---
 2 files changed, 38 insertions(+), 19 deletions(-)

Comments

Markus Armbruster Sept. 16, 2020, 3:13 p.m. UTC | #1
John Snow <jsnow@redhat.com> writes:

> Code style tools really dislike the use of global keywords, because it
> generally involves re-binding the name at runtime which can have strange
> effects depending on when and how that global name is referenced in
> other modules.
>
> Make a little indent level manager instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/common.py | 50 +++++++++++++++++++++++++++++-------------
>  scripts/qapi/visit.py  |  7 +++---
>  2 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 4fb265a8bf..87d87b95e5 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -93,33 +93,53 @@ def c_name(name, protect=True):
>  pointer_suffix = ' *' + eatspace
>  
>  
> -def genindent(count):
> -    ret = ''
> -    for _ in range(count):
> -        ret += ' '
> -    return ret
> +class Indent:

Since this class name will be used just once...  Indentation or
IndentationManager?

> +    """
> +    Indent-level management.
>  
> +    :param initial: Initial value, default 0.

The only caller passes 0.

Let's drop the parameter, and hardcode the default initial value until
we have an actual use for another initial value.

> +    """
> +    def __init__(self, initial: int = 0) -> None:
> +        self._level = initial
>  
> -indent_level = 0
> +    def __int__(self) -> int:
> +        """Return the indent as an integer."""

Isn't "as an integer" obvious?

Let's replace "the indent" by "the indentation" globally.

> +        return self._level
>  
> +    def __repr__(self) -> str:
> +        return "{}({:d})".format(type(self).__name__, self._level)

Is __repr__ needed?

>  
> -def push_indent(indent_amount=4):
> -    global indent_level
> -    indent_level += indent_amount
> +    def __str__(self) -> str:
> +        """Return the indent as a string."""
> +        return ' ' * self._level
>  
> +    def __bool__(self) -> bool:
> +        """True when there is a non-zero indent."""
> +        return bool(self._level)

This one lets us shorten

    if int(INDENT):

to

    if INDENT:

There's just one instance.  Marginal.  I'm not rejecting it.

> -def pop_indent(indent_amount=4):
> -    global indent_level
> -    indent_level -= indent_amount
> +    def push(self, amount: int = 4) -> int:
> +        """Push `amount` spaces onto the indent, default four."""
> +        self._level += amount
> +        return self._level
> +
> +    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)))
> +        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.

The @amount parameter has never been used.  I don't mind keeping it.

> +
> +
> +INDENT = Indent(0)

Uh, does this really qualify as a constant?  It looks quite variable to
me...

>  # Generate @code with @kwds interpolated.
>  # Obey indent_level, and strip eatspace.
>  def cgen(code, **kwds):
>      raw = code % kwds
> -    if indent_level:
> -        indent = genindent(indent_level)
> -        raw, _ = re.subn(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
> +    if INDENT:
> +        raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>      return re.sub(re.escape(eatspace) + r' *', '', raw)
>  
>  
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 31bf46e7f7..66ce3dc52e 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -19,8 +19,7 @@
>      gen_endif,
>      gen_if,
>      mcgen,
> -    pop_indent,
> -    push_indent,
> +    INDENT,
>  )
>  from .gen import QAPISchemaModularCVisitor, ifcontext
>  from .schema import QAPISchemaObjectType
> @@ -68,7 +67,7 @@ def gen_visit_object_members(name, base, members, variants):
>      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
>  ''',
>                           name=memb.name, c_name=c_name(memb.name))
> -            push_indent()
> +            INDENT.push()
>          ret += mcgen('''
>      if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
>          return false;
> @@ -77,7 +76,7 @@ def gen_visit_object_members(name, base, members, variants):
>                       c_type=memb.type.c_name(), name=memb.name,
>                       c_name=c_name(memb.name))
>          if memb.optional:
> -            pop_indent()
> +            INDENT.pop()
>              ret += mcgen('''
>      }
>  ''')
John Snow Sept. 16, 2020, 10:25 p.m. UTC | #2
On 9/16/20 11:13 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:

> 

>> Code style tools really dislike the use of global keywords, because it

>> generally involves re-binding the name at runtime which can have strange

>> effects depending on when and how that global name is referenced in

>> other modules.

>>

>> Make a little indent level manager instead.

>>

>> Signed-off-by: John Snow <jsnow@redhat.com>

>> ---

>>   scripts/qapi/common.py | 50 +++++++++++++++++++++++++++++-------------

>>   scripts/qapi/visit.py  |  7 +++---

>>   2 files changed, 38 insertions(+), 19 deletions(-)

>>

>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py

>> index 4fb265a8bf..87d87b95e5 100644

>> --- a/scripts/qapi/common.py

>> +++ b/scripts/qapi/common.py

>> @@ -93,33 +93,53 @@ def c_name(name, protect=True):

>>   pointer_suffix = ' *' + eatspace

>>   

>>   

>> -def genindent(count):

>> -    ret = ''

>> -    for _ in range(count):

>> -        ret += ' '

>> -    return ret

>> +class Indent:

> 

> Since this class name will be used just once...  Indentation or

> IndentationManager?

> 


Indentation is fine, if you'd like. IndentationManager makes me feel 
ashamed for writing this patch, like I am punishing myself with Java.

>> +    """

>> +    Indent-level management.

>>   

>> +    :param initial: Initial value, default 0.

> 

> The only caller passes 0.

> 

> Let's drop the parameter, and hardcode the default initial value until

> we have an actual use for another initial value.

> 


The parameter is nice because it gives meaning to the output of repr(), 
see below.

>> +    """

>> +    def __init__(self, initial: int = 0) -> None:

>> +        self._level = initial

>>   

>> -indent_level = 0

>> +    def __int__(self) -> int:

>> +        """Return the indent as an integer."""

> 

> Isn't "as an integer" obvious?


Just a compulsion to write complete sentences that got lost in the sea 
of many patches. I'll nix this one, because dunder methods do not need 
to be documented.

> 

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

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

>>   

>> -def push_indent(indent_amount=4):

>> -    global indent_level

>> -    indent_level += indent_amount

>> +    def __str__(self) -> str:

>> +        """Return the indent as a string."""

>> +        return ' ' * self._level

>>   

>> +    def __bool__(self) -> bool:

>> +        """True when there is a non-zero indent."""

>> +        return bool(self._level)

> 

> This one lets us shorten

> 

>      if int(INDENT):

> 

> to

> 

>      if INDENT:

> 

> There's just one instance.  Marginal.  I'm not rejecting it.

> 


Yep, it's trivial in either direction. Still, because it's a custom 
type, I thought I'd be courteous and have it support bool().

>> -def pop_indent(indent_amount=4):

>> -    global indent_level

>> -    indent_level -= indent_amount

>> +    def push(self, amount: int = 4) -> int:

>> +        """Push `amount` spaces onto the indent, default four."""

>> +        self._level += amount

>> +        return self._level

>> +

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

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

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

>>   # Generate @code with @kwds interpolated.

>>   # Obey indent_level, and strip eatspace.

>>   def cgen(code, **kwds):

>>       raw = code % kwds

>> -    if indent_level:

>> -        indent = genindent(indent_level)

>> -        raw, _ = re.subn(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)

>> +    if INDENT:

>> +        raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)

>>       return re.sub(re.escape(eatspace) + r' *', '', raw)

>>   

>>   

>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py

>> index 31bf46e7f7..66ce3dc52e 100644

>> --- a/scripts/qapi/visit.py

>> +++ b/scripts/qapi/visit.py

>> @@ -19,8 +19,7 @@

>>       gen_endif,

>>       gen_if,

>>       mcgen,

>> -    pop_indent,

>> -    push_indent,

>> +    INDENT,

>>   )

>>   from .gen import QAPISchemaModularCVisitor, ifcontext

>>   from .schema import QAPISchemaObjectType

>> @@ -68,7 +67,7 @@ def gen_visit_object_members(name, base, members, variants):

>>       if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {

>>   ''',

>>                            name=memb.name, c_name=c_name(memb.name))

>> -            push_indent()

>> +            INDENT.push()

>>           ret += mcgen('''

>>       if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {

>>           return false;

>> @@ -77,7 +76,7 @@ def gen_visit_object_members(name, base, members, variants):

>>                        c_type=memb.type.c_name(), name=memb.name,

>>                        c_name=c_name(memb.name))

>>           if memb.optional:

>> -            pop_indent()

>> +            INDENT.pop()

>>               ret += mcgen('''

>>       }

>>   ''')
Markus Armbruster Sept. 17, 2020, 8:07 a.m. UTC | #3
John Snow <jsnow@redhat.com> writes:

> On 9/16/20 11:13 AM, Markus Armbruster wrote:

>> John Snow <jsnow@redhat.com> writes:

>> 

>>> Code style tools really dislike the use of global keywords, because it

>>> generally involves re-binding the name at runtime which can have strange

>>> effects depending on when and how that global name is referenced in

>>> other modules.

>>>

>>> Make a little indent level manager instead.

>>>

>>> Signed-off-by: John Snow <jsnow@redhat.com>

>>> ---

>>>   scripts/qapi/common.py | 50 +++++++++++++++++++++++++++++-------------

>>>   scripts/qapi/visit.py  |  7 +++---

>>>   2 files changed, 38 insertions(+), 19 deletions(-)

>>>

>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py

>>> index 4fb265a8bf..87d87b95e5 100644

>>> --- a/scripts/qapi/common.py

>>> +++ b/scripts/qapi/common.py

>>> @@ -93,33 +93,53 @@ def c_name(name, protect=True):

>>>   pointer_suffix = ' *' + eatspace

>>>     

>>> -def genindent(count):

>>> -    ret = ''

>>> -    for _ in range(count):

>>> -        ret += ' '

>>> -    return ret

>>> +class Indent:

>> Since this class name will be used just once...  Indentation or

>> IndentationManager?

>> 

>

> Indentation is fine, if you'd like. IndentationManager makes me feel

> ashamed for writing this patch, like I am punishing myself with Java.


Hah!  Point taken.

>>> +    """

>>> +    Indent-level management.

>>>   +    :param initial: Initial value, default 0.

>> The only caller passes 0.

>> Let's drop the parameter, and hardcode the default initial value

>> until

>> we have an actual use for another initial value.

>> 

>

> The parameter is nice because it gives meaning to the output of

> repr(), see below.

>

>>> +    """

>>> +    def __init__(self, initial: int = 0) -> None:

>>> +        self._level = initial

>>>   -indent_level = 0

>>> +    def __int__(self) -> int:

>>> +        """Return the indent as an integer."""

>> Isn't "as an integer" obvious?

>

> Just a compulsion to write complete sentences that got lost in the sea

> of many patches. I'll nix this one, because dunder methods do not need 

> to be documented.

>

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

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

>>>   -def push_indent(indent_amount=4):

>>> -    global indent_level

>>> -    indent_level += indent_amount

>>> +    def __str__(self) -> str:

>>> +        """Return the indent as a string."""

>>> +        return ' ' * self._level

>>>   +    def __bool__(self) -> bool:

>>> +        """True when there is a non-zero indent."""

>>> +        return bool(self._level)

>> This one lets us shorten

>>      if int(INDENT):

>> to

>>      if INDENT:

>> There's just one instance.  Marginal.  I'm not rejecting it.

>> 

>

> Yep, it's trivial in either direction. Still, because it's a custom

> type, I thought I'd be courteous and have it support bool().

>

>>> -def pop_indent(indent_amount=4):

>>> -    global indent_level

>>> -    indent_level -= indent_amount

>>> +    def push(self, amount: int = 4) -> int:

>>> +        """Push `amount` spaces onto the indent, default four."""

>>> +        self._level += amount

>>> +        return self._level

>>> +

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

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

>> 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!"
John Snow Sept. 17, 2020, 5:18 p.m. UTC | #4
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!"
> 

:)
Markus Armbruster Sept. 18, 2020, 10:55 a.m. UTC | #5
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.)

[...]
John Snow Sept. 18, 2020, 4:08 p.m. UTC | #6
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
Markus Armbruster Sept. 21, 2020, 7:43 a.m. UTC | #7
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?" :)
diff mbox series

Patch

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 4fb265a8bf..87d87b95e5 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -93,33 +93,53 @@  def c_name(name, protect=True):
 pointer_suffix = ' *' + eatspace
 
 
-def genindent(count):
-    ret = ''
-    for _ in range(count):
-        ret += ' '
-    return ret
+class Indent:
+    """
+    Indent-level management.
 
+    :param initial: Initial value, default 0.
+    """
+    def __init__(self, initial: int = 0) -> None:
+        self._level = initial
 
-indent_level = 0
+    def __int__(self) -> int:
+        """Return the indent as an integer."""
+        return self._level
 
+    def __repr__(self) -> str:
+        return "{}({:d})".format(type(self).__name__, self._level)
 
-def push_indent(indent_amount=4):
-    global indent_level
-    indent_level += indent_amount
+    def __str__(self) -> str:
+        """Return the indent as a string."""
+        return ' ' * self._level
 
+    def __bool__(self) -> bool:
+        """True when there is a non-zero indent."""
+        return bool(self._level)
 
-def pop_indent(indent_amount=4):
-    global indent_level
-    indent_level -= indent_amount
+    def push(self, amount: int = 4) -> int:
+        """Push `amount` spaces onto the indent, default four."""
+        self._level += amount
+        return self._level
+
+    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)))
+        self._level -= amount
+        return self._level
+
+
+INDENT = Indent(0)
 
 
 # Generate @code with @kwds interpolated.
 # Obey indent_level, and strip eatspace.
 def cgen(code, **kwds):
     raw = code % kwds
-    if indent_level:
-        indent = genindent(indent_level)
-        raw, _ = re.subn(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
+    if INDENT:
+        raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
     return re.sub(re.escape(eatspace) + r' *', '', raw)
 
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 31bf46e7f7..66ce3dc52e 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -19,8 +19,7 @@ 
     gen_endif,
     gen_if,
     mcgen,
-    pop_indent,
-    push_indent,
+    INDENT,
 )
 from .gen import QAPISchemaModularCVisitor, ifcontext
 from .schema import QAPISchemaObjectType
@@ -68,7 +67,7 @@  def gen_visit_object_members(name, base, members, variants):
     if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
 ''',
                          name=memb.name, c_name=c_name(memb.name))
-            push_indent()
+            INDENT.push()
         ret += mcgen('''
     if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
         return false;
@@ -77,7 +76,7 @@  def gen_visit_object_members(name, base, members, variants):
                      c_type=memb.type.c_name(), name=memb.name,
                      c_name=c_name(memb.name))
         if memb.optional:
-            pop_indent()
+            INDENT.pop()
             ret += mcgen('''
     }
 ''')