Re: [PATCH v4 04/18] tests/qapi/doc-good.json: Clean up markup

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 5 Aug 2020 at 14:03, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> The subject is a bit misleading.  The markup doesn't need cleanup.  It
>> purposefully tests corner cases of the doc comment parser.  For some of
>> them, the conversion to rST will change the meaning.  This commit tweaks
>> the test so it passes before and after the conversion.  Makes it a worse
>> test for the doc comment parser, which doesn't matter, because the code
>> in question is about to be deleted.
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > doc-good.json tests some oddities of markup that we don't want to
>> > accept.  Make them match the more restrictive rST syntax:
>> >
>> >  * in a single list the bullet types must all match
>> >  * lists must have leading and following blank lines
>> >  * indentation is important
>>
>> Actually, indentation has always been important, but the conversion to
>> rST changes where and how it matters.
>
> Mmm. More specifically, indentation was previously unimportant
> within a multiline block of text, and is now important there.
>
>> >  * the '|' example syntax is going to go away entirely, so stop
>> >    testing it
>>
>> Before the series, we (try to) cover all doc markup here.
>>
>> The series replaces the doc markup language by rST + a little extra.
>> The test must still cover the little extra then, and covering a bit of
>> rST to ensure basic sanity won't hurt.
>>
>> Correct?
>>
>> I'm asking because a "yes" explains why we can drop coverage without
>> replacement: it's appropriate when the markup in question is replaced by
>> rST.
>
> I guess so. We still want some test coverage of the stuff the
> doc-comment parser is doing that's actually looking for
> arguments and so on; we don't really need to check that
> rst is rst.
>
> It's been a while since I wrote this patch, but basically IIRC
> it's "make the minimal changes necessary so that the test does
> not start failing for the parser changes that will follow".

That's okay, but the commit message could be clearer.  Perhaps:

    tests/qapi/doc-good.json: Prepare for qapi-doc Sphinx extension

    doc-good.json tests doc comment parser corner cases.  We're about to
    largely replace it by a Sphinx extension, which will have different
    corner cases.  Tweak the test so it passes both with the old parser
    and the Sphinx extension, by making it match the more restrictive
    rST syntax:

     * in a single list the bullet types must all match
     * lists must have leading and following blank lines
     * indentation is important
     * the '|' example syntax is going to go away entirely, so stop
       testing it

    This will avoid the tests spuriously breaking when we tighten up the
    parser code in the following commits.

>> > This will avoid the tests spuriously breaking when we tighten up the
>> > parser code in the following commits.
>
>> > @@ -62,7 +62,7 @@
>> >  ##
>> >  # @Base:
>> >  # @base1:
>>
>> Here, indentation is relevant even before the series: @base: is only
>> recognized as an argument section when it's not indented.
>>
>> > -#   the first member
>> > +# the first member
>>
>> Why do you need to unindent this line?
>
> Because in the tightened syntax, you can either have:
>
> @foo: line 1
>       line 2
>
> (the definition part of the argument is multiple lines of
> rST, which all must be lined up to start at the same column)
>
> or
>
> @foo:
> line1
> line2
>
> (the definition is multiple lines of rST, which all start
> in column 1)
>
> But
> @foo:
>    line1
>
> is neither of the above, and will be invalid.
>
> The old parser simply stripped all the leading whitespace
> from this kind of multiline lump-of-doc-text. That doesn't
> work for rST because we want to be able to have doc-text
> where leading whitespace matters. So we need to know how
> many characters of whitespace to delete from each line. The
> two options above basically correspond to the two major
> pre-existing kinds of doc-comment.
>
> Compare commit 26ec4e53f2bf0a381, which fixed up the
> doc comments in qapi/ proper to follow this "one of
> these two indent models, but not anything else" pattern.

Ah, now I remember.

Remind me, how is messed up doc comment indentation reported in the
brave new Sphinx world?

>> >  ##
>> >  { 'struct': 'Base', 'data': { 'base1': 'Enum' } }
>> >
>> > @@ -101,7 +101,7 @@
>> >  ##
>> >  # @Alternate:
>> >  # @i: an integer
>> > -# @b is undocumented
>> > +#     @b is undocumented
>>
>> Why do you need to indent this line?
>
> Again, because it needs to follow one of the two
> indent patterns described above. Either no text
> following the "@i:" and all lines start in column 1,
> or all lines have to start in the same column as
> the text following the "@i:". In this case I went for
> option 2.
>
> The test input is a bit odd because it's talking
> about @b in the description-text for @i, but there
> you go. You could alternatively add a newline
> after the @i: line to put the text "@b is undocumented"
> into the doc-text for the whole @Alternate struct
> rather than into the doc-text for the @i member.

The test's purpose is to be mean to the doc parser, not pleasant to the
reader's eye ;)

The doc before the patch tests the (less than ideal) handling of a
mistake that is easy to make, namely forgetting the ':' after the member
name.

Your change is okay.

>> >  { 'alternate': 'Alternate',
>> >    'data': { 'i': 'int', 'b': 'bool' } }
>> > @@ -115,7 +115,7 @@
>> >  # @arg1: the first argument
>> >  #
>> >  # @arg2: the second
>> > -# argument
>> > +#        argument
>>
>> Why do you need to indent this line?
>
> As above, beacuse it's not in either of the
> two standard "this is what indent for a multi-line
> bit of doc text for an argument can be" patterns.

Thanks!

Preferably with an improved commit message
Reviewed-by: Markus Armbruster <armbru@redhat.com>