| Message ID | 20201005195158.2348217-12-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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(''' > } > ''')
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(''' >> } >> ''')
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
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.
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 --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(''' } ''')