John Snow <js...@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 <js...@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('''
> }
> ''')
