[PATCH v2 13/38] qapi/common.py: add type hint annotations

John Snow posted 38 patches 5 years, 4 months ago
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Cleber Rosa <crosa@redhat.com>
There is a newer version of this series
[PATCH v2 13/38] qapi/common.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago
Annotations do not change runtime behavior.
This commit *only* adds annotations.

Signed-off-by: John Snow <jsnow@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 682e74fe65..0ce4a107e6 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',
@@ -133,24 +136,24 @@ def decrease(self, amount: int = 4) -> int:
 
 # 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
@@ -159,7 +162,7 @@ def guardstart(name):
                  name=c_fname(name).upper())
 
 
-def guardend(name):
+def guardend(name: str) -> str:
     return mcgen('''
 
 #endif /* %(name)s */
@@ -167,7 +170,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('''
@@ -176,7 +179,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('''
@@ -185,7 +188,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:
-- 
2.26.2


Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
Posted by Eduardo Habkost 5 years, 4 months ago
On Tue, Sep 22, 2020 at 05:00:36PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
> 
> Signed-off-by: John Snow <jsnow@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 682e74fe65..0ce4a107e6 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,6 +12,7 @@
[...]
> @@ -176,7 +179,7 @@ def gen_if(ifcond):
>      return ret
>  
>  
> -def gen_endif(ifcond):
> +def gen_endif(ifcond: Sequence[str]) -> str:

Does this need to require a Sequence?  It looks like it could be
Iterable.

I don't think this should block the patch, though, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>      ret = ''
>      for ifc in reversed(ifcond):
>          ret += mcgen('''
> @@ -185,7 +188,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:
> -- 
> 2.26.2
> 

-- 
Eduardo


Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago
On 9/22/20 6:44 PM, Eduardo Habkost wrote:
> Does this need to require a Sequence?  It looks like it could be
> Iterable.
> 
> I don't think this should block the patch, though, so:
> 
> Reviewed-by: Eduardo Habkost<ehabkost@redhat.com>

gen_if can take an Iterator, gen_endif needs a Sequence because it uses 
reversed().

I have not been very fastidious about choosing the MOST possibly 
abstracted type, but you are right that we SHOULD. I was hoping to find 
all occurrences of List and try slackening them to 
Collection/Sequence/Iterable etc, but as a follow-up.

In this case, since gen_endif actually does want the stricter type, I'm 
deciding to leave them as Sequence for signature parity.

--js

(P.S. in case it comes up in future review, there is a bug that prevents 
us from using Collection in 3.6: 
https://github.com/PyCQA/pylint/issues/2377 )


Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
Posted by Eduardo Habkost 5 years, 4 months ago
On Wed, Sep 23, 2020 at 01:57:25PM -0400, John Snow wrote:
> On 9/22/20 6:44 PM, Eduardo Habkost wrote:
> > Does this need to require a Sequence?  It looks like it could be
> > Iterable.
> > 
> > I don't think this should block the patch, though, so:
> > 
> > Reviewed-by: Eduardo Habkost<ehabkost@redhat.com>
> 
> gen_if can take an Iterator, gen_endif needs a Sequence because it uses
> reversed().
> 
> I have not been very fastidious about choosing the MOST possibly abstracted
> type, but you are right that we SHOULD. I was hoping to find all occurrences
> of List and try slackening them to Collection/Sequence/Iterable etc, but as
> a follow-up.
> 
> In this case, since gen_endif actually does want the stricter type, I'm
> deciding to leave them as Sequence for signature parity.

I hadn't noticed reversed() requires a Sequence.  I agree using
the same type on both is better.  Thanks!

-- 
Eduardo


Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
Posted by Cleber Rosa 5 years, 4 months ago
On Tue, Sep 22, 2020 at 05:00:36PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>