Re: [PATCH v5 24/36] qapi/source.py: add type hint annotations

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> A note on typing of __init__: mypy requires init functions with no
> parameters to document a return type of None to be considered fully
> typed. In the case when there are input parameters, None may be omitted.
>
> Since __init__ may never return any value, it is preferred to omit the
> return annotation whenever possible.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qapi/mypy.ini  |  5 -----
>  scripts/qapi/source.py | 31 ++++++++++++++++++-------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 8ab9ac52cc4..1b8555dfa39 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -34,11 +34,6 @@ disallow_untyped_defs = False
>  disallow_incomplete_defs = False
>  check_untyped_defs = False
>  
> -[mypy-qapi.source]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
>  [mypy-qapi.types]
>  disallow_untyped_defs = False
>  disallow_incomplete_defs = False
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index e97b9a8e15e..1cc6a5b82dc 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -11,37 +11,42 @@
>  
>  import copy
>  import sys
> +from typing import List, Optional, TypeVar
>  
>  
>  class QAPISchemaPragma:
> -    def __init__(self):
> +    def __init__(self) -> None:
>          # Are documentation comments required?
>          self.doc_required = False
>          # Whitelist of commands allowed to return a non-dictionary
> -        self.returns_whitelist = []
> +        self.returns_whitelist: List[str] = []
>          # Whitelist of entities allowed to violate case conventions
> -        self.name_case_whitelist = []
> +        self.name_case_whitelist: List[str] = []
>  
>  
>  class QAPISourceInfo:
> -    def __init__(self, fname, line, parent):
> +    T = TypeVar('T', bound='QAPISourceInfo')
> +
> +    def __init__(self: T, fname: str, line: int, parent: Optional[T]):

More ignorant questions (I'm abusing the review process to learn Python
type hints)...

Why do you need to annotate self here, but not elsewhere?

Why do you use T instead of QAPISourceInfo?

>          self.fname = fname
>          self.line = line
>          self.parent = parent
> -        self.pragma = parent.pragma if parent else QAPISchemaPragma()
> -        self.defn_meta = None
> -        self.defn_name = None
> +        self.pragma: QAPISchemaPragma = (
> +            parent.pragma if parent else QAPISchemaPragma()
> +        )

Type inference fail?

> +        self.defn_meta: Optional[str] = None
> +        self.defn_name: Optional[str] = None
>  
> -    def set_defn(self, meta, name):
> +    def set_defn(self, meta: str, name: str) -> None:
>          self.defn_meta = meta
>          self.defn_name = name
>  
> -    def next_line(self):
> +    def next_line(self: T) -> T:
>          info = copy.copy(self)
>          info.line += 1
>          return info
>  
> -    def loc(self):
> +    def loc(self) -> str:
>          if self.fname is None:
>              return sys.argv[0]
>          ret = self.fname
> @@ -49,13 +54,13 @@ def loc(self):
>              ret += ':%d' % self.line
>          return ret
>  
> -    def in_defn(self):
> +    def in_defn(self) -> str:
>          if self.defn_name:
>              return "%s: In %s '%s':\n" % (self.fname,
>                                            self.defn_meta, self.defn_name)
>          return ''
>  
> -    def include_path(self):
> +    def include_path(self) -> str:
>          ret = ''
>          parent = self.parent
>          while parent:
> @@ -63,5 +68,5 @@ def include_path(self):
>              parent = parent.parent
>          return ret
>  
> -    def __str__(self):
> +    def __str__(self) -> str:
>          return self.include_path() + self.in_defn() + self.loc()