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

[John Snow <jsnow@redhat.com>]

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.