Re: [PATCH v4 06/14] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is only used to pass in a dictionary with a comment already set, so
> skip the runaround and just accept the comment.
>
> This works because _tree_to_qlit() treats 'if': None; 'comment': None
> exactly like absent 'if'; 'comment'.

Confusing, because the two paragraphs talk about two different things:

1. Actual arguments for @extra are either None or {'comment': comment}.
   Simplify: replace parameter @extra by parameter @comment.

2. Dumb down the return value to always be of the form

    (obj {'if': ifcond, 'comment': comment})

I suspect splitting the patch is easier than crafting a clear commit
message for the combined one.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index d3fbf694ad2..0aa3b77109f 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,6 +10,8 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> +from typing import Optional
> +
>  from .common import (
>      c_name,
>      gen_endif,
> @@ -24,11 +26,11 @@
>  )
>  
>  
> -def _make_tree(obj, ifcond, extra=None):
> -    if extra is None:
> -        extra = {}
> -    if ifcond:
> -        extra['if'] = ifcond
> +def _make_tree(obj, ifcond, comment=None):
> +    extra = {
> +        'if': ifcond,
> +        'comment': comment,
> +    }
>      return (obj, extra)

Obvious way to do just 1.:

   def _make_tree(obj, ifcond, comment=None):
       extra = {}
       if ifcond:
           extra['if'] = ifcond
       if comment:
           extra['comment'] = comment

>  
>  
> @@ -174,18 +176,18 @@ def _gen_features(features):
>          return [_make_tree(f.name, f.ifcond) for f in features]
>  
>      def _gen_tree(self, name, mtype, obj, ifcond, features):
> -        extra = None
> +        comment: Optional[str] = None
>          if mtype not in ('command', 'event', 'builtin', 'array'):
>              if not self._unmask:
>                  # Output a comment to make it easy to map masked names
>                  # back to the source when reading the generated output.
> -                extra = {'comment': '"%s" = %s' % (self._name(name), name)}
> +                comment = f'"{self._name(name)}" = {name}'
>              name = self._name(name)
>          obj['name'] = name
>          obj['meta-type'] = mtype
>          if features:
>              obj['features'] = self._gen_features(features)
> -        self._trees.append(_make_tree(obj, ifcond, extra))
> +        self._trees.append(_make_tree(obj, ifcond, comment))
>  
>      def _gen_member(self, member):
>          obj = {'name': member.name, 'type': self._use_type(member.type)}