Re: [Qemu-devel] [PATCH for-4.0 v7 22/27] qapi: add 'If:' condition to enum values documentation

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Tue, Dec 11, 2018 at 8:00 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Use a common function to generate the "If:..." line.
>> >
>> > While at it, get rid of the existing \n\n (no idea why it was
>> > there). Use a line-break in member description, this seems to look
>> > slightly better in the plaintext version.
>>
>> Where exactly in the patch is this done?
>
> -    if ifcond:
> -        body += '\n\n@b{If:} @code{%s}' % ", ".join(ifcond)
> +    body += texi_if(ifcond, suffix='')

Thanks.  I suspected that much, but wasn't sure.

> See also doc-good.texi output
>
>>
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  scripts/qapi/doc.py             | 24 +++++++++++++++---------
>> >  tests/qapi-schema/doc-good.json |  4 +++-
>> >  tests/qapi-schema/doc-good.out  |  1 +
>> >  tests/qapi-schema/doc-good.texi |  2 +-
>> >  4 files changed, 20 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
>> > index 76cb186ff9..2133ded47e 100755
>> > --- a/scripts/qapi/doc.py
>> > +++ b/scripts/qapi/doc.py
>> > @@ -126,19 +126,26 @@ def texi_body(doc):
>> >      return texi_format(doc.body.text)
>> >
>> >
>> > -def texi_enum_value(value):
>> > +def texi_if(ifcond, prefix='\n', suffix='\n'):
>> > +    """Format the #if condition"""
>> > +    return '%s@b{If:} @code{%s}%s' % (
>> > +        prefix, ', '.join(ifcond), suffix) if ifcond else ''
>>
>> I like the ternary operator, but I nevertheless think this function is
>> easier to read as
>>
>>        if not ifcond:
>>            return ''
>>        return '%s@b{If:} @code{%s}%s' % (prefix, ', '.join(ifcond), suffix)
>>
>
> No difference to me, sure
>
>> > +
>> > +
>> > +def texi_enum_value(value, desc, suffix=''):
>> >      """Format a table of members item for an enumeration value"""
>> > -    return '@item @code{%s}\n' % value.name
>> > +    return '@item @code{%s}\n%s%s' % (
>> > +        value.name, desc, texi_if(value.ifcond, prefix='@*'))
>>
>> Do you ignore suffix intentionally?
>
> yes

That's kind of surprising, until you pry open the abstraction and
realize that we reach this one only from the first call of member_func()
below, which ...

>>
>> >
>> >
>> > -def texi_member(member, suffix=''):
>> > +def texi_member(member, desc='', suffix=''):
>> >      """Format a table of members item for an object type member"""
>> >      typ = member.type.doc_type()
>> >      membertype = ': ' + typ if typ else ''
>> > -    return '@item @code{%s%s}%s%s\n' % (
>> > +    return '@item @code{%s%s}%s%s\n%s' % (
>> >          member.name, membertype,
>> >          ' (optional)' if member.optional else '',
>> > -        suffix)
>> > +        suffix, desc)
>> >
>> >
>> >  def texi_members(doc, what, base, variants, member_func):
>> > @@ -155,7 +162,7 @@ def texi_members(doc, what, base, variants, member_func):
>> >              desc = 'One of ' + members_text + '\n'
>> >          else:
>> >              desc = 'Not documented\n'
>> > -        items += member_func(section.member) + desc
>> > +        items += member_func(section.member, desc)
>>
>> Here, you pass a @desc argument, but no @suffix.
>>
>> @member_func is either texi_enum_value() or texi_member().  Works.

... doesn't pass @suffix.  So @suffix is empty when we ignore it.

>> >      if base:
>> >          items += '@item The members of @code{%s}\n' % base.doc_type()
>> >      if variants:
>> > @@ -165,7 +172,7 @@ def texi_members(doc, what, base, variants, member_func):
>> >              if v.type.is_implicit():
>> >                  assert not v.type.base and not v.type.variants
>> >                  for m in v.type.local_members:
>> > -                    items += member_func(m, when)
>> > +                    items += member_func(m, suffix=when)
>>
>> Here, you pass a @suffix argument, but no @desc.
>>
>> texi_enum_value() would choke on that, but it can't occur here, because
>> enums have no variants.
>>
>> Still, I'd prefer texi_enum_value() and texi_member() to have the exact
>> same signature.
>
> OK (not sure what is the more pythonic way)

I suspect there's a clean solution struggling to get out.  But I can't
grasp it yet.

>>
>> >              else:
>> >                  items += '@item The members of @code{%s}%s\n' % (
>> >                      v.type.doc_type(), when)
>> > @@ -185,8 +192,7 @@ def texi_sections(doc, ifcond):
>> >              body += texi_example(section.text)
>> >          else:
>> >              body += texi_format(section.text)
>> > -    if ifcond:
>> > -        body += '\n\n@b{If:} @code{%s}' % ", ".join(ifcond)
>> > +    body += texi_if(ifcond, suffix='')
>> >      return body
>> >
>> >
[...]