On 2/16/21 4:10 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Optional[List] is clunky; an empty sequence can more elegantly convey
>> "no variants". By downgrading "List" to "Sequence", we can also accept
>> tuples; this is useful for the empty tuple specifically, which we may
>> use as a default parameter because it is immutable.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 649225988d1..e4d31a59503 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -247,13 +247,13 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>> return self._name(typ.name)
>>
>> @staticmethod
>> - def _gen_features(features: List[QAPISchemaFeature]
>> + def _gen_features(features: Sequence[QAPISchemaFeature]
>> ) -> List[Annotated[str]]:
>> return [Annotated(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> ifcond: Sequence[str],
>> - features: Optional[List[QAPISchemaFeature]]) -> None:
>> + features: Sequence[QAPISchemaFeature] = ()) -> None:
>> """
>> Build and append a SchemaInfo object to self._trees.
>>
>> @@ -261,7 +261,8 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> :param mtype: The entity's meta-type.
>> :param obj: Additional entity fields, as appropriate for the meta-type.
>> :param ifcond: Sequence of conditionals that apply to this entity.
>> - :param features: Optional features field for SchemaInfo.
>> + :param features: Optional, The features field for SchemaInfo.
>
> Downcase "The".
>
>> + Will be omitted from the output if empty.
>> """
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> @@ -298,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>>
>> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> json_type: str) -> None:
>> - self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>> + self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
>>
>> def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> @@ -316,7 +317,7 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> element_type: QAPISchemaType) -> None:
>> element = self._use_type(element_type)
>> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> - ifcond, None)
>> + ifcond)
>>
>> def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>
> Marginal by itself. Might be worth it as part of a more general move
> away from Optional[List[...]]. See also next patch.
>
Yep, it's tiny. Still, maybe worth taking just to remove a bad habit
from the code in case of cargo-culting?
Go with whatcha feel, it's a style nit you raised -- I'm not sure I'll
remember to do a more thorough pass after pt6 is done.
ACK to change the comment casing if you take these.
--js