Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations

[John Snow]

On 9/23/20 6:36 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:00:45PM -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/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 9da1dccef4..43c8bd1973 100644
> > --- a/scripts/qapi/mypy.ini
> > +++ b/scripts/qapi/mypy.ini
> > @@ -39,11 +39,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
> > -
>
> This is what I meant in my comment in the previous patch.  It looks
> like a mix of commit grannurality styles.  Not a blocker though.

Yep. Just how the chips fell. Some files were just very quick to cleanup 
and I didn't have to refactor them much when I split things out, so the 
enablements got rolled in.

I will, once reviews are in (and there is a commitment to merge), try to 
squash things where it seems appropriate.

> >   [mypy-qapi.types]
> >   disallow_untyped_defs = False
> >   disallow_incomplete_defs = False
> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> > index e97b9a8e15..1cc6a5b82d 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:
>
> I don't follow the reason for typing this...
>
> >           # 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]):
>
> And not this... to tune my review approach, should I assume that this
> series intends to add complete type hints or not?

This is a mypy quirk you've discovered that I've simply forgotten about.

When __init__ has *no* arguments, you need to annotate its return to 
explain to mypy that you have fully typed that method. It's a sentinel 
that says "Please type check this class!"

When __init__ has some arguments, you only need to type those arguments 
and not the return type. The sentinel is not needed.

__init__ *never* returns anything, so I opted to omit this useless 
annotation whenever it was possible to do so.

> - Cleber.