| Message ID | 20200309154405.13548-5-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert QAPI doc comments to generate rST instead of texinfo | expand |
On 3/9/20 8:43 AM, Peter Maydell wrote: > 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 > * 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. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > New patch in v2 > --- > tests/qapi-schema/doc-good.json | 25 +++++++++++++------------ > tests/qapi-schema/doc-good.out | 12 ++++++------ > tests/qapi-schema/doc-good.texi | 12 +++--------- > 3 files changed, 22 insertions(+), 27 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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. > * 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. > This will avoid the tests spuriously breaking when we tighten up the > parser code in the following commits. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > New patch in v2 > --- > tests/qapi-schema/doc-good.json | 25 +++++++++++++------------ > tests/qapi-schema/doc-good.out | 12 ++++++------ > tests/qapi-schema/doc-good.texi | 12 +++--------- > 3 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json > index d992e713d97..a45dc22848c 100644 > --- a/tests/qapi-schema/doc-good.json > +++ b/tests/qapi-schema/doc-good.json > @@ -10,25 +10,25 @@ > # > # *strong* _with emphasis_ > # @var {in braces} > +# > # * List item one > -# - Two, multiple > +# * Two, multiple > # lines > # > -# 3. Three > -# Still in list > +# * Three > +# Still in list > +# > +# Not in list > # > -# Not in list This is an example for indentation becoming relevant where it wasn't before. The old doc markup language uses blank lines to terminate list items. The new one uses indentation, which is more flexible. > # - Second list > -# Note: still in list > +# Note: still in list > # > # Note: not in list > +# > # 1. Third list > # is numbered > # > -# - another item > -# > -# | example > -# | multiple lines > +# 2. another item > # > # Returns: the King > # Since: the first age > @@ -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? > ## > { '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? > ## > { '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? > # > # Features: > # @cmd-feat1: a feature > @@ -124,6 +124,7 @@ > # Returns: @Object > # TODO: frobnicate > # Notes: > +# > # - Lorem ipsum dolor sit amet > # - Ut enim ad minim veniam > # > diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out > index 4c9406a464d..9503a3a3178 100644 > --- a/tests/qapi-schema/doc-good.out > +++ b/tests/qapi-schema/doc-good.out > @@ -68,25 +68,25 @@ doc freeform > > *strong* _with emphasis_ > @var {in braces} > + > * List item one > -- Two, multiple > +* Two, multiple > lines > > -3. Three > +* Three > Still in list > > Not in list > + > - Second list > Note: still in list > > Note: not in list > + > 1. Third list > is numbered > > -- another item > - > -| example > -| multiple lines > +2. another item > > Returns: the King > Since: the first age > diff --git a/tests/qapi-schema/doc-good.texi b/tests/qapi-schema/doc-good.texi > index d4b15dabf03..1e8063c8579 100644 > --- a/tests/qapi-schema/doc-good.texi > +++ b/tests/qapi-schema/doc-good.texi > @@ -6,6 +6,7 @@ > > @strong{strong} @emph{with emphasis} > @code{var} @{in braces@} > + > @itemize @bullet > @item > List item one > @@ -20,6 +21,7 @@ Still in list > @end itemize > > Not in list > + > @itemize @minus > @item > Second list > @@ -28,6 +30,7 @@ Note: still in list > @end itemize > > Note: not in list > + > @enumerate > @item > Third list > @@ -36,15 +39,6 @@ is numbered > @item > another item > > -@example > -example > -@end example > - > -@example > -multiple lines > -@end example > - > - > @end enumerate > > Returns: the King
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". > > 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. > > ## > > { '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. > > { '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 -- PMM
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>
On Thu, 6 Aug 2020 at 15:46, Markus Armbruster <armbru@redhat.com> wrote: > Remind me, how is messed up doc comment indentation reported in the > brave new Sphinx world? For insufficient indentatation, like this: GEN qapi-gen In file included from /home/petmay01/linaro/qemu-from-laptop/qemu/qapi/qapi-schema.json:91: /home/petmay01/linaro/qemu-from-laptop/qemu/qapi/machine.json:194:1: unexpected de-indent (where the quoted line number in the .json file is the line that was insufficiently indented). If you over-indent, then the QAPI parser assumes you intended to use some rST syntax which needed the extra indent, and passes it to Sphinx, which will either interpret it as per the rST spec or complain. thanks -- PMM
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json index d992e713d97..a45dc22848c 100644 --- a/tests/qapi-schema/doc-good.json +++ b/tests/qapi-schema/doc-good.json @@ -10,25 +10,25 @@ # # *strong* _with emphasis_ # @var {in braces} +# # * List item one -# - Two, multiple +# * Two, multiple # lines # -# 3. Three -# Still in list +# * Three +# Still in list +# +# Not in list # -# Not in list # - Second list -# Note: still in list +# Note: still in list # # Note: not in list +# # 1. Third list # is numbered # -# - another item -# -# | example -# | multiple lines +# 2. another item # # Returns: the King # Since: the first age @@ -62,7 +62,7 @@ ## # @Base: # @base1: -# the first member +# the first member ## { 'struct': 'Base', 'data': { 'base1': 'Enum' } } @@ -101,7 +101,7 @@ ## # @Alternate: # @i: an integer -# @b is undocumented +# @b is undocumented ## { 'alternate': 'Alternate', 'data': { 'i': 'int', 'b': 'bool' } } @@ -115,7 +115,7 @@ # @arg1: the first argument # # @arg2: the second -# argument +# argument # # Features: # @cmd-feat1: a feature @@ -124,6 +124,7 @@ # Returns: @Object # TODO: frobnicate # Notes: +# # - Lorem ipsum dolor sit amet # - Ut enim ad minim veniam # diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 4c9406a464d..9503a3a3178 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -68,25 +68,25 @@ doc freeform *strong* _with emphasis_ @var {in braces} + * List item one -- Two, multiple +* Two, multiple lines -3. Three +* Three Still in list Not in list + - Second list Note: still in list Note: not in list + 1. Third list is numbered -- another item - -| example -| multiple lines +2. another item Returns: the King Since: the first age diff --git a/tests/qapi-schema/doc-good.texi b/tests/qapi-schema/doc-good.texi index d4b15dabf03..1e8063c8579 100644 --- a/tests/qapi-schema/doc-good.texi +++ b/tests/qapi-schema/doc-good.texi @@ -6,6 +6,7 @@ @strong{strong} @emph{with emphasis} @code{var} @{in braces@} + @itemize @bullet @item List item one @@ -20,6 +21,7 @@ Still in list @end itemize Not in list + @itemize @minus @item Second list @@ -28,6 +30,7 @@ Note: still in list @end itemize Note: not in list + @enumerate @item Third list @@ -36,15 +39,6 @@ is numbered @item another item -@example -example -@end example - -@example -multiple lines -@end example - - @end enumerate Returns: the King
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 * 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. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- New patch in v2 --- tests/qapi-schema/doc-good.json | 25 +++++++++++++------------ tests/qapi-schema/doc-good.out | 12 ++++++------ tests/qapi-schema/doc-good.texi | 12 +++--------- 3 files changed, 22 insertions(+), 27 deletions(-) -- 2.20.1