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

[Marc-André Lureau]

Hi

On Tue, Dec 11, 2018 at 8:00 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> 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 += 'address@hidden:} @code{%s}' % ", ".join(ifcond)
+    body += texi_if(ifcond, suffix='')

See also doc-good.texi output

>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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 'address@hidden:} @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 'address@hidden:} @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

>
> >
> >
> > -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.
>
> >      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)

>
> >              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 += 'address@hidden:} @code{%s}' % ", ".join(ifcond)
> > +    body += texi_if(ifcond, suffix='')
> >      return body
> >
> >
> > diff --git a/tests/qapi-schema/doc-good.json 
> > b/tests/qapi-schema/doc-good.json
> > index 984cd8ed06..c7fe08c530 100644
> > --- a/tests/qapi-schema/doc-good.json
> > +++ b/tests/qapi-schema/doc-good.json
> > @@ -55,7 +55,9 @@
> >  #
> >  # @two is undocumented
> >  ##
> > -{ 'enum': 'Enum', 'data': [ 'one', 'two' ], 'if': 'defined(IFCOND)' }
> > +{ 'enum': 'Enum', 'data':
> > +  [ { 'name': 'one', 'if': 'defined(IFENUM)' }, 'two' ],
> > +  'if': 'defined(IFCOND)' }
>
> IFCOND applies to the enum, and IFENUM applies to one of its values.
> Awkward.  Let's rename IFENUM to IFONE.

ok

>
> >
> >  ##
> >  # @Base:
> > diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> > index c2fc5c774a..a05535b69b 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -11,6 +11,7 @@ enum QType
> >  module doc-good.json
> >  enum Enum
> >      member one
> > +        if ['defined(IFENUM)']
> >      member two
> >      if ['defined(IFCOND)']
> >  object Base
> > diff --git a/tests/qapi-schema/doc-good.texi 
> > b/tests/qapi-schema/doc-good.texi
> > index e42eace474..5c0231e0e6 100644
> > --- a/tests/qapi-schema/doc-good.texi
> > +++ b/tests/qapi-schema/doc-good.texi
> > @@ -84,12 +84,12 @@ Examples:
> >  @table @asis
> >  @item @code{one}
> >  The @emph{one} @{and address@hidden
> > address@hidden@b{If:} @code{defined(IFENUM)}
>
> @* forces a line break.  Putting it at the end of the previous line
> would be tidier, but let's ignore that for now.

For consistency and easier reading, I think it's better to keep it as
a seperate line in the end for the corresponding section

>
> >  @item @code{two}
> >  Not documented
> >  @end table
> >  @code{two} is undocumented
> >
> > -
> >  @b{If:} @code{defined(IFCOND)}
> >  @end deftp

thanks