Re: [PATCH v5 27/36] qapi/gen.py: add type hint annotations

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 10/7/20 9:20 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/gen.py | 104 ++++++++++++++++++++++++--------------------
>>>   1 file changed, 57 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 1bad37fc06b..d0391cd8718 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -17,7 +17,13 @@
>>>   import errno
>>>   import os
>>>   import re
>>> -from typing import Optional
>>> +from typing import (
>>> +    Dict,
>>> +    Iterator,
>>> +    List,
>>> +    Optional,
>>> +    Tuple,
>>> +)
>>>     from .common import (
>>>       c_fname,
>>> @@ -29,31 +35,31 @@
>>>       mcgen,
>>>   )
>>>   from .schema import QAPISchemaObjectType, QAPISchemaVisitor
>>> +from .source import QAPISourceInfo
>> PATCH 03 has a similar cleanup.  Are there more?  Perhaps a separate
>> patch doing just this kind of cleanup would make sense.  Up to you.
>> [...]
>> 
>
> This isn't a cleanup, I am just importing QAPISourceInfo to use for an
> annotation. It's relevant and required for this patch, and doesn't
> make sense on its own.

I was mistaken.  A case of patch-review-eye...

> Patch 03 ... Oh, you mean identifying the correct location of
> QAPIError. Uh... nah? I think that was the only case of that one
> changing. Not worth pulling out or naming, I think.

Agree.