diff mbox series

[v5,11/36] qapi/common.py: Add indent manager

Message ID 20201005195158.2348217-12-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Oct. 5, 2020, 7:51 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qapi/common.py | 49 ++++++++++++++++++++++++++++--------------
 scripts/qapi/visit.py  |  7 +++---
 2 files changed, 36 insertions(+), 20 deletions(-)

Comments

Markus Armbruster Oct. 7, 2020, 8:50 a.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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>

Intentation is a job for QAPIGen (and its subtypes).  But if this patch
is easier to achieve this series' goal, I don't mind.

> ---
>  scripts/qapi/common.py | 49 ++++++++++++++++++++++++++++--------------
>  scripts/qapi/visit.py  |  7 +++---
>  2 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index cee63eb95c7..b35318b72cf 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -93,33 +93,50 @@ def c_name(name, protect=True):
>  pointer_suffix = ' *' + eatspace
>  
>  
> -def genindent(count):
> -    ret = ''
> -    for _ in range(count):
> -        ret += ' '
> -    return ret
> +class Indentation:
> +    """
> +    Indentation level management.
>  
> +    :param initial: Initial number of spaces, default 0.
> +    """
> +    def __init__(self, initial: int = 0) -> None:
> +        self._level = initial
>  
> -indent_level = 0
> +    def __int__(self) -> int:
> +        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 current indentation as a string of spaces."""
> +        return ' ' * self._level
>  
> +    def __bool__(self) -> bool:
> +        """True when there is a non-zero indentation."""
> +        return bool(self._level)
>  
> -def pop_indent(indent_amount=4):
> -    global indent_level
> -    indent_level -= indent_amount
> +    def increase(self, amount: int = 4) -> None:
> +        """Increase the indentation level by ``amount``, default 4."""
> +        self._level += amount
> +
> +    def decrease(self, amount: int = 4) -> None:
> +        """Decrease the indentation level by ``amount``, default 4."""
> +        if self._level < amount:
> +            raise ArithmeticError(
> +                f"Can't remove {amount:d} spaces from {self!r}")

Raise a fancy error when there's an actual need for it.  You're not
coding a framework thousands of people you never heard of will put to
uses you cannot imagine.

> +        self._level -= amount
> +
> +
> +indent = Indentation()
>  
>  
>  # Generate @code with @kwds interpolated.
> -# Obey indent_level, and strip eatspace.
> +# Obey indent, and strip eatspace.
>  def cgen(code, **kwds):
>      raw = code % kwds
> -    if indent_level:
> -        indent = genindent(indent_level)
> -        raw = re.sub(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
> +    if indent:
> +        raw = re.sub(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 808410d6f1b..14f30c228b7 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -18,9 +18,8 @@
>      c_name,
>      gen_endif,
>      gen_if,
> +    indent,
>      mcgen,
> -    pop_indent,
> -    push_indent,
>  )
>  from .gen import QAPISchemaModularCVisitor, ifcontext
>  from .schema import QAPISchemaObjectType
> @@ -69,7 +68,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.increase()
>          ret += mcgen('''
>      if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
>          return false;
> @@ -78,7 +77,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.decrease()
>              ret += mcgen('''
>      }
>  ''')
John Snow Oct. 7, 2020, 6:08 p.m. UTC | #2
On 10/7/20 4:50 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>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 
> Intentation is a job for QAPIGen (and its subtypes).  But if this patch
> is easier to achieve this series' goal, I don't mind.
> 

I agree, but refactoring it properly is beyond my capacity right now.

This was the dumbest thing I could do to get pylint/mypy passing, which 
required the elimination (or suppression) of the global keyword.

Creating a stateful object was the fastest way from A to B.

>> ---
>>   scripts/qapi/common.py | 49 ++++++++++++++++++++++++++++--------------
>>   scripts/qapi/visit.py  |  7 +++---
>>   2 files changed, 36 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index cee63eb95c7..b35318b72cf 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -93,33 +93,50 @@ def c_name(name, protect=True):
>>   pointer_suffix = ' *' + eatspace
>>   
>>   
>> -def genindent(count):
>> -    ret = ''
>> -    for _ in range(count):
>> -        ret += ' '
>> -    return ret
>> +class Indentation:
>> +    """
>> +    Indentation level management.
>>   
>> +    :param initial: Initial number of spaces, default 0.
>> +    """
>> +    def __init__(self, initial: int = 0) -> None:
>> +        self._level = initial
>>   
>> -indent_level = 0
>> +    def __int__(self) -> int:
>> +        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 current indentation as a string of spaces."""
>> +        return ' ' * self._level
>>   
>> +    def __bool__(self) -> bool:
>> +        """True when there is a non-zero indentation."""
>> +        return bool(self._level)
>>   
>> -def pop_indent(indent_amount=4):
>> -    global indent_level
>> -    indent_level -= indent_amount
>> +    def increase(self, amount: int = 4) -> None:
>> +        """Increase the indentation level by ``amount``, default 4."""
>> +        self._level += amount
>> +
>> +    def decrease(self, amount: int = 4) -> None:
>> +        """Decrease the indentation level by ``amount``, default 4."""
>> +        if self._level < amount:
>> +            raise ArithmeticError(
>> +                f"Can't remove {amount:d} spaces from {self!r}")
> 
> Raise a fancy error when there's an actual need for it.  You're not
> coding a framework thousands of people you never heard of will put to
> uses you cannot imagine.
> 

It's not fancy, it's just a normal built-in exception, like 
AssertionError or any other:

Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
ArithmeticError: Can't remove 4 spaces from Indent(0)

vs

Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
AssertionError: Can't remove 4 spaces from Indent(0)


I feel like it's kind of a lateral move? I realize you feel this is an 
overfancy class doing what we hope is a temporary job, and that I have 
overengineered the hell out of a tiny do-nothing class... but I suppose 
that's also why I feel weird changing it around so much to accomplish so 
little.

Differences in style, I suppose.

Feel free to change it around to suit your tastes, I don't think it's 
worth spending a lot of ping-pong time on this paintsink in particular.

--js

>> +        self._level -= amount
>> +
>> +
>> +indent = Indentation()
>>   
>>   
>>   # Generate @code with @kwds interpolated.
>> -# Obey indent_level, and strip eatspace.
>> +# Obey indent, and strip eatspace.
>>   def cgen(code, **kwds):
>>       raw = code % kwds
>> -    if indent_level:
>> -        indent = genindent(indent_level)
>> -        raw = re.sub(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
>> +    if indent:
>> +        raw = re.sub(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 808410d6f1b..14f30c228b7 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -18,9 +18,8 @@
>>       c_name,
>>       gen_endif,
>>       gen_if,
>> +    indent,
>>       mcgen,
>> -    pop_indent,
>> -    push_indent,
>>   )
>>   from .gen import QAPISchemaModularCVisitor, ifcontext
>>   from .schema import QAPISchemaObjectType
>> @@ -69,7 +68,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.increase()
>>           ret += mcgen('''
>>       if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
>>           return false;
>> @@ -78,7 +77,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.decrease()
>>               ret += mcgen('''
>>       }
>>   ''')
Eduardo Habkost Oct. 7, 2020, 6:18 p.m. UTC | #3
On Wed, Oct 07, 2020 at 02:08:33PM -0400, John Snow wrote:
> On 10/7/20 4:50 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>

> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> > > Reviewed-by: Cleber Rosa <crosa@redhat.com>

> > 

> > Intentation is a job for QAPIGen (and its subtypes).  But if this patch

> > is easier to achieve this series' goal, I don't mind.

> > 

> 

> I agree, but refactoring it properly is beyond my capacity right now.

> 

> This was the dumbest thing I could do to get pylint/mypy passing, which

> required the elimination (or suppression) of the global keyword.

> 

> Creating a stateful object was the fastest way from A to B.


An even dumber solution could be:

  indent_prefixes = []
  def push_indent(amount=4):
      """Add `amount` spaces to indentation prefix"""
      indent_prefixes.push(' '*amount)
  def pop_indent():
      """Revert the most recent push_indent() call"""
      indent_prefixes.pop()
  def genindent():
      """Return the current indentation prefix"""
      return ''.join(indent_prefixes)

No global keyword involved, and the only stateful object is a
dumb list.

-- 
Eduardo
Markus Armbruster Oct. 8, 2020, 7:23 a.m. UTC | #4
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Oct 07, 2020 at 02:08:33PM -0400, John Snow wrote:
>> On 10/7/20 4:50 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>
>> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> > > Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> > 
>> > Intentation is a job for QAPIGen (and its subtypes).  But if this patch
>> > is easier to achieve this series' goal, I don't mind.
>> > 
>> 
>> I agree, but refactoring it properly is beyond my capacity right now.
>> 
>> This was the dumbest thing I could do to get pylint/mypy passing, which
>> required the elimination (or suppression) of the global keyword.
>> 
>> Creating a stateful object was the fastest way from A to B.
>
> An even dumber solution could be:
>
>   indent_prefixes = []
>   def push_indent(amount=4):
>       """Add `amount` spaces to indentation prefix"""
>       indent_prefixes.push(' '*amount)
>   def pop_indent():
>       """Revert the most recent push_indent() call"""
>       indent_prefixes.pop()
>   def genindent():
>       """Return the current indentation prefix"""
>       return ''.join(indent_prefixes)
>
> No global keyword involved, and the only stateful object is a
> dumb list.

Ha, this is Dumb with a capital D!  I respect that :)

John, I'm not asking you to switch.  Use your judgement.
John Snow Oct. 8, 2020, 5:45 p.m. UTC | #5
On 10/8/20 3:23 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Wed, Oct 07, 2020 at 02:08:33PM -0400, John Snow wrote:
>>> On 10/7/20 4:50 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>
>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>>>
>>>> Intentation is a job for QAPIGen (and its subtypes).  But if this patch
>>>> is easier to achieve this series' goal, I don't mind.
>>>>
>>>
>>> I agree, but refactoring it properly is beyond my capacity right now.
>>>
>>> This was the dumbest thing I could do to get pylint/mypy passing, which
>>> required the elimination (or suppression) of the global keyword.
>>>
>>> Creating a stateful object was the fastest way from A to B.
>>
>> An even dumber solution could be:
>>
>>    indent_prefixes = []
>>    def push_indent(amount=4):
>>        """Add `amount` spaces to indentation prefix"""
>>        indent_prefixes.push(' '*amount)
>>    def pop_indent():
>>        """Revert the most recent push_indent() call"""
>>        indent_prefixes.pop()
>>    def genindent():
>>        """Return the current indentation prefix"""
>>        return ''.join(indent_prefixes)
>>
>> No global keyword involved, and the only stateful object is a
>> dumb list.
> 
> Ha, this is Dumb with a capital D!  I respect that :)
> 
> John, I'm not asking you to switch.  Use your judgement.
> 

It's something we'll revisit soon, I'm sure. it's not good to be 
managing indent in so many different ways in so many different places.

(I prefer to leave it alone for now to try and press forward with 
accomplishing regression checks on strict typing.)

--js
diff mbox series

Patch

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index cee63eb95c7..b35318b72cf 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -93,33 +93,50 @@  def c_name(name, protect=True):
 pointer_suffix = ' *' + eatspace
 
 
-def genindent(count):
-    ret = ''
-    for _ in range(count):
-        ret += ' '
-    return ret
+class Indentation:
+    """
+    Indentation level management.
 
+    :param initial: Initial number of spaces, default 0.
+    """
+    def __init__(self, initial: int = 0) -> None:
+        self._level = initial
 
-indent_level = 0
+    def __int__(self) -> int:
+        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 current indentation as a string of spaces."""
+        return ' ' * self._level
 
+    def __bool__(self) -> bool:
+        """True when there is a non-zero indentation."""
+        return bool(self._level)
 
-def pop_indent(indent_amount=4):
-    global indent_level
-    indent_level -= indent_amount
+    def increase(self, amount: int = 4) -> None:
+        """Increase the indentation level by ``amount``, default 4."""
+        self._level += amount
+
+    def decrease(self, amount: int = 4) -> None:
+        """Decrease the indentation level by ``amount``, default 4."""
+        if self._level < amount:
+            raise ArithmeticError(
+                f"Can't remove {amount:d} spaces from {self!r}")
+        self._level -= amount
+
+
+indent = Indentation()
 
 
 # Generate @code with @kwds interpolated.
-# Obey indent_level, and strip eatspace.
+# Obey indent, and strip eatspace.
 def cgen(code, **kwds):
     raw = code % kwds
-    if indent_level:
-        indent = genindent(indent_level)
-        raw = re.sub(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE)
+    if indent:
+        raw = re.sub(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 808410d6f1b..14f30c228b7 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -18,9 +18,8 @@ 
     c_name,
     gen_endif,
     gen_if,
+    indent,
     mcgen,
-    pop_indent,
-    push_indent,
 )
 from .gen import QAPISchemaModularCVisitor, ifcontext
 from .schema import QAPISchemaObjectType
@@ -69,7 +68,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.increase()
         ret += mcgen('''
     if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
         return false;
@@ -78,7 +77,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.decrease()
             ret += mcgen('''
     }
 ''')