| Message ID | 20200915224027.2529813-10-jsnow@redhat.com |
|---|---|
| State | New |
| 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> > --- > 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(''' > } > ''')
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(''' >> } >> ''')
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!"
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!" > :)
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.) [...]
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
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 --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(''' } ''')
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(-)