Re: [PATCH v4 3/6] qapi: Add minor typing workaround for 3.6

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> Pylint under 3.6 does not believe that Collection is subscriptable at
>> runtime. It is, making this a Pylint
>> bug. https://github.com/PyCQA/pylint/issues/2377
>>
>> They closed it as fixed, but that doesn't seem to be true as of Pylint
>> 2.13.9, the latest version you can install under Python 3.6. 2.13.9 was
>> released 2022-05-13, about seven months after the bug was closed.
>>
>> The least-annoying fix here is to just use the more specific type
>> Sequence, only because it seems to work in 3.6.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  scripts/qapi/expr.py | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 5a1782b57ea..8701351fdfc 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -33,11 +33,11 @@
>>  
>>  import re
>>  from typing import (
>> -    Collection,
>>      Dict,
>>      Iterable,
>>      List,
>>      Optional,
>> +    Sequence,
>>      Union,
>>      cast,
>>  )
>> @@ -195,8 +195,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>>  def check_keys(value: _JSONObject,
>>                 info: QAPISourceInfo,
>>                 source: str,
>> -               required: Collection[str],
>> -               optional: Collection[str]) -> None:
>> +               required: Sequence[str],
>> +               optional: Sequence[str]) -> None:
>>      """
>>      Ensure that a dict has a specific set of keys.
>
> The actual arguments are always List[str].  You actually used that until
> v3 of the patch, and switched to the maximally general Collection[str]
> in v4, with rationale that ended up in commit 538cd41065a:
>
>     qapi/expr.py: Modify check_keys to accept any Collection
>     
>     This is a minor adjustment that lets parameters @required and
>     @optional take tuple arguments, in particular ().  Later patches will
>     make use of that.
>
> No later patch ever did.
>
> I'd prefer maximally stupid List[str], but it's no big deal either way.

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>