Re: [PATCH v5 15/36] qapi/common.py: add type hint annotations

[John Snow]

On 10/7/20 5:03 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Annotations do not change runtime behavior.
> > This commit *only* adds annotations.
> >
> > 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 | 27 ++++++++++++++++-----------
> >   1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 338adedef4f..74a2c001ed9 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -12,6 +12,7 @@
> >   # See the COPYING file in the top-level directory.
> >   
> >   import re
> > +from typing import Optional, Sequence
> >   
> >   
> >   EATSPACE = '\033EATSPACE.'
> > @@ -22,7 +23,7 @@
> >   # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
> >   # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
> >   # ENUM24_Name -> ENUM24_NAME
> > -def camel_to_upper(value):
> > +def camel_to_upper(value: str) -> str:
> >       c_fun_str = c_name(value, False)
> >       if value.isupper():
> >           return c_fun_str
> > @@ -41,7 +42,9 @@ def camel_to_upper(value):
> >       return new_name.lstrip('_').upper()
> >   
> >   
> > -def c_enum_const(type_name, const_name, prefix=None):
> > +def c_enum_const(type_name: str,
> > +                 const_name: str,
> > +                 prefix: Optional[str] = None) -> str:
> >       if prefix is not None:
> >           type_name = prefix
> >       return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
> > @@ -56,7 +59,7 @@ def c_enum_const(type_name, const_name, prefix=None):
> >   # into substrings of a generated C function name.
> >   # '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
> >   # protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
> > -def c_name(name, protect=True):
> > +def c_name(name: str, protect: bool = True) -> str:
> >       # ANSI X3J11/88-090, 3.1.1
> >       c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
> >                        'default', 'do', 'double', 'else', 'enum', 'extern',
> > @@ -131,24 +134,24 @@ def decrease(self, amount: int = 4) -> None:
> >   
> >   # Generate @code with @kwds interpolated.
> >   # Obey indent, and strip EATSPACE.
> > -def cgen(code, **kwds):
> > +def cgen(code: str, **kwds: object) -> str:
> >       raw = code % kwds
> >       if indent:
> >           raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE)
> >       return re.sub(re.escape(EATSPACE) + r' *', '', raw)
> >   
> >   
> > -def mcgen(code, **kwds):
> > +def mcgen(code: str, **kwds: object) -> str:
> >       if code[0] == '\n':
> >           code = code[1:]
> >       return cgen(code, **kwds)
> >   
> >   
> > -def c_fname(filename):
> > +def c_fname(filename: str) -> str:
> >       return re.sub(r'[^A-Za-z0-9_]', '_', filename)
> >   
> >   
> > -def guardstart(name):
> > +def guardstart(name: str) -> str:
> >       return mcgen('''
> >   #ifndef %(name)s
> >   #define %(name)s
> > @@ -157,7 +160,7 @@ def guardstart(name):
> >                    name=c_fname(name).upper())
> >   
> >   
> > -def guardend(name):
> > +def guardend(name: str) -> str:
> >       return mcgen('''
> >   
> >   #endif /* %(name)s */
> > @@ -165,7 +168,7 @@ def guardend(name):
> >                    name=c_fname(name).upper())
> >   
> >   
> > -def gen_if(ifcond):
> > +def gen_if(ifcond: Sequence[str]) -> str:
> >       ret = ''
> >       for ifc in ifcond:
> >           ret += mcgen('''
> > @@ -174,7 +177,7 @@ def gen_if(ifcond):
> >       return ret
> >   
> >   
> > -def gen_endif(ifcond):
> > +def gen_endif(ifcond: Sequence[str]) -> str:
> >       ret = ''
> >       for ifc in reversed(ifcond):
> >           ret += mcgen('''
> > @@ -183,7 +186,9 @@ def gen_endif(ifcond):
> >       return ret
> >   
> >   
> > -def build_params(arg_type, boxed, extra=None):
> > +def build_params(arg_type,
> > +                 boxed: bool,
> > +                 extra: Optional[str] = None) -> str:
> >       ret = ''
> >       sep = ''
> >       if boxed:
>
> @arg_type is the only parameter left unannotated.  Scratching head...
> aha:
>
>      qapi/common.py: move build_params into gen.py
>      
>      Including it in common.py creates a circular import dependency; schema
>      relies on common, but common.build_params requires a type annotation
>      from schema. To type this properly, it needs to be moved outside the
>      cycle.
>
> Let's amend the commit message:
>
>      Note that build_params() cannot be fully annotated due to import
>      dependency issues.  The commit after next will take care of it.
>
> If we swap the next two commits, the fix follows immediately.  Better,
> but only worth it if swapping is pretty much effortless.

The reason they're not flipped was to avoid giving the impression that 
docstrings were what was preventing us from enabling the mypy type checking.

The reason I don't flip the comments and the type hints is because 
that's a lot of rebase pain for perceptibly little benefit.

I amended the commit message.