qapi: Doc comment parsing & doc generation work

[PATCH 11/16] qapi: Recognize section tags and 'Features:' only after blank line

Posted by Markus Armbruster 8 months, 2 weeks ago

Putting a blank line before section tags and 'Features:' is good,
existing practice.  Enforce it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.rst                 | 15 +++++++++------
 scripts/qapi/parser.py                       | 11 ++++++++---
 tests/qapi-schema/doc-duplicated-return.err  |  2 +-
 tests/qapi-schema/doc-duplicated-return.json |  1 +
 tests/qapi-schema/doc-duplicated-since.err   |  2 +-
 tests/qapi-schema/doc-duplicated-since.json  |  1 +
 tests/qapi-schema/doc-good.json              |  9 +++++++++
 tests/qapi-schema/doc-invalid-return.err     |  2 +-
 tests/qapi-schema/doc-invalid-return.json    |  1 +
 9 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 77a40f3bdc..6804a4b596 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -986,16 +986,17 @@ indented like this::
 Extensions added after the definition was first released carry a
 "(since x.y.z)" comment.
 
-The feature descriptions must be preceded by a line "Features:", like
-this::
+The feature descriptions must be preceded by a blank line and then a
+line "Features:", like this::
 
+  #
   # Features:
   #
   # @feature: Description text
 
-A tagged section starts with one of the following words:
-"Note:"/"Notes:", "Since:", "Example:"/"Examples:", "Returns:",
-"TODO:".  The section ends with the start of a new section.
+A tagged section begins with a paragraph that starts with one of the
+following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:",
+"Returns:", "TODO:".  It ends with the start of a new section.
 
 The second and subsequent lines of tagged sections must be indented
 like this::
@@ -1086,8 +1087,10 @@ need to line up with each other, like this::
  #     or cache associativity unknown)
  #     (since 5.0)
 
-Section tags are case-sensitive and end with a colon.  Good example::
+Section tags are case-sensitive and end with a colon.  They are only
+recognized after a blank line.  Good example::
 
+ #
  # Since: 7.1
 
 Bad examples (all ordinary paragraphs)::
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f8da315332..de2ce3ec2c 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -538,6 +538,7 @@ def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
         # the current section
         self._section = self.body
         self._append_line = self._append_body_line
+        self._first_line_in_paragraph = False
 
     def has_section(self, tag: str) -> bool:
         """Return True if we have a section with this tag."""
@@ -560,12 +561,14 @@ def append(self, line: str) -> None:
         line = line[1:]
         if not line:
             self._append_freeform(line)
+            self._first_line_in_paragraph = True
             return
 
         if line[0] != ' ':
             raise QAPIParseError(self._parser, "missing space after #")
         line = line[1:]
         self._append_line(line)
+        self._first_line_in_paragraph = False
 
     def end_comment(self) -> None:
         self._switch_section(QAPIDoc.NullSection(self._parser))
@@ -574,9 +577,11 @@ def end_comment(self) -> None:
     def _match_at_name_colon(string: str) -> Optional[Match[str]]:
         return re.match(r'@([^:]*): *', string)
 
-    @staticmethod
-    def _match_section_tag(string: str) -> Optional[Match[str]]:
-        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
+    def _match_section_tag(self, string: str) -> Optional[Match[str]]:
+        if not self._first_line_in_paragraph:
+            return None
+        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *',
+                        string)
 
     def _append_body_line(self, line: str) -> None:
         """
diff --git a/tests/qapi-schema/doc-duplicated-return.err b/tests/qapi-schema/doc-duplicated-return.err
index fe97e3db8d..f19a2b8ec4 100644
--- a/tests/qapi-schema/doc-duplicated-return.err
+++ b/tests/qapi-schema/doc-duplicated-return.err
@@ -1 +1 @@
-doc-duplicated-return.json:7:1: duplicated 'Returns' section
+doc-duplicated-return.json:8:1: duplicated 'Returns' section
diff --git a/tests/qapi-schema/doc-duplicated-return.json b/tests/qapi-schema/doc-duplicated-return.json
index b44b5ae979..4e1ec2ef42 100644
--- a/tests/qapi-schema/doc-duplicated-return.json
+++ b/tests/qapi-schema/doc-duplicated-return.json
@@ -4,5 +4,6 @@
 # @foo:
 #
 # Returns: 0
+#
 # Returns: 1
 ##
diff --git a/tests/qapi-schema/doc-duplicated-since.err b/tests/qapi-schema/doc-duplicated-since.err
index abca141a2c..565b753b6a 100644
--- a/tests/qapi-schema/doc-duplicated-since.err
+++ b/tests/qapi-schema/doc-duplicated-since.err
@@ -1 +1 @@
-doc-duplicated-since.json:7:1: duplicated 'Since' section
+doc-duplicated-since.json:8:1: duplicated 'Since' section
diff --git a/tests/qapi-schema/doc-duplicated-since.json b/tests/qapi-schema/doc-duplicated-since.json
index 343cd872cb..2755f95719 100644
--- a/tests/qapi-schema/doc-duplicated-since.json
+++ b/tests/qapi-schema/doc-duplicated-since.json
@@ -4,5 +4,6 @@
 # @foo:
 #
 # Since: 0
+#
 # Since: 1
 ##
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 977bb38b6e..5bb2b69071 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -154,22 +154,29 @@
 # Features:
 # @cmd-feat1: a feature
 # @cmd-feat2: another feature
+#
 # Note: @arg3 is undocumented
+#
 # Returns: @Object
+#
 # TODO: frobnicate
+#
 # Notes:
 #
 #  - Lorem ipsum dolor sit amet
 #  - Ut enim ad minim veniam
 #
 #  Duis aute irure dolor
+#
 # Example:
 #
 #  -> in
 #  <- out
+#
 # Examples:
 #  - *verbatim*
 #  - {braces}
+#
 # Since: 2.10
 ##
 { 'command': 'cmd',
@@ -180,9 +187,11 @@
 ##
 # @cmd-boxed:
 # If you're bored enough to read this, go see a video of boxed cats
+#
 # Features:
 # @cmd-feat1: a feature
 # @cmd-feat2: another feature
+#
 # Example:
 #
 #  -> in
diff --git a/tests/qapi-schema/doc-invalid-return.err b/tests/qapi-schema/doc-invalid-return.err
index bc5826de20..3d9e71c2b3 100644
--- a/tests/qapi-schema/doc-invalid-return.err
+++ b/tests/qapi-schema/doc-invalid-return.err
@@ -1 +1 @@
-doc-invalid-return.json:5: 'Returns:' is only valid for commands
+doc-invalid-return.json:6: 'Returns:' is only valid for commands
diff --git a/tests/qapi-schema/doc-invalid-return.json b/tests/qapi-schema/doc-invalid-return.json
index 95e7583930..1aabef3482 100644
--- a/tests/qapi-schema/doc-invalid-return.json
+++ b/tests/qapi-schema/doc-invalid-return.json
@@ -2,6 +2,7 @@
 
 ##
 # @FOO:
+#
 # Returns: blah
 ##
 { 'event': 'FOO' }
-- 
2.43.0

Re: [PATCH 11/16] qapi: Recognize section tags and 'Features:' only after blank line

Posted by Daniel P. Berrangé 8 months, 2 weeks ago

On Fri, Feb 16, 2024 at 03:58:35PM +0100, Markus Armbruster wrote:
> Putting a blank line before section tags and 'Features:' is good,
> existing practice.  Enforce it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/devel/qapi-code-gen.rst                 | 15 +++++++++------
>  scripts/qapi/parser.py                       | 11 ++++++++---
>  tests/qapi-schema/doc-duplicated-return.err  |  2 +-
>  tests/qapi-schema/doc-duplicated-return.json |  1 +
>  tests/qapi-schema/doc-duplicated-since.err   |  2 +-
>  tests/qapi-schema/doc-duplicated-since.json  |  1 +
>  tests/qapi-schema/doc-good.json              |  9 +++++++++
>  tests/qapi-schema/doc-invalid-return.err     |  2 +-
>  tests/qapi-schema/doc-invalid-return.json    |  1 +
>  9 files changed, 32 insertions(+), 12 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> @@ -574,9 +577,11 @@ def end_comment(self) -> None:
>      def _match_at_name_colon(string: str) -> Optional[Match[str]]:
>          return re.match(r'@([^:]*): *', string)
>  
> -    @staticmethod
> -    def _match_section_tag(string: str) -> Optional[Match[str]]:
> -        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
> +    def _match_section_tag(self, string: str) -> Optional[Match[str]]:
> +        if not self._first_line_in_paragraph:
> +            return None
> +        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *',
> +                        string)

I guess I have a minor worry that we're silently ignoring
these tags when there's no blank line. Could result in
docs silently rendering in the wrong way if (when) someone
forgets the blank line.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 11/16] qapi: Recognize section tags and 'Features:' only after blank line

Posted by Markus Armbruster 8 months, 2 weeks ago

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Feb 16, 2024 at 03:58:35PM +0100, Markus Armbruster wrote:
>> Putting a blank line before section tags and 'Features:' is good,
>> existing practice.  Enforce it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/devel/qapi-code-gen.rst                 | 15 +++++++++------
>>  scripts/qapi/parser.py                       | 11 ++++++++---
>>  tests/qapi-schema/doc-duplicated-return.err  |  2 +-
>>  tests/qapi-schema/doc-duplicated-return.json |  1 +
>>  tests/qapi-schema/doc-duplicated-since.err   |  2 +-
>>  tests/qapi-schema/doc-duplicated-since.json  |  1 +
>>  tests/qapi-schema/doc-good.json              |  9 +++++++++
>>  tests/qapi-schema/doc-invalid-return.err     |  2 +-
>>  tests/qapi-schema/doc-invalid-return.json    |  1 +
>>  9 files changed, 32 insertions(+), 12 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
>> @@ -574,9 +577,11 @@ def end_comment(self) -> None:
>>      def _match_at_name_colon(string: str) -> Optional[Match[str]]:
>>          return re.match(r'@([^:]*): *', string)
>>  
>> -    @staticmethod
>> -    def _match_section_tag(string: str) -> Optional[Match[str]]:
>> -        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
>> +    def _match_section_tag(self, string: str) -> Optional[Match[str]]:
>> +        if not self._first_line_in_paragraph:
>> +            return None
>> +        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *',
>> +                        string)
>
> I guess I have a minor worry that we're silently ignoring
> these tags when there's no blank line. Could result in
> docs silently rendering in the wrong way if (when) someone
> forgets the blank line.

True.

We could make it an error.  Can flag occurences in the middle of a
free-form paragraph that happen to be at the beginning of a line, but
that seems unlikely.

Of course, rST is full of similar syntactic traps anyway.  To quote
Paolo, "I find it to be the Perl of ASCII-based markups."

Re: [PATCH 11/16] qapi: Recognize section tags and 'Features:' only after blank line

Posted by Daniel P. Berrangé 8 months, 2 weeks ago

On Tue, Feb 20, 2024 at 05:06:42PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Feb 16, 2024 at 03:58:35PM +0100, Markus Armbruster wrote:
> >> Putting a blank line before section tags and 'Features:' is good,
> >> existing practice.  Enforce it.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  docs/devel/qapi-code-gen.rst                 | 15 +++++++++------
> >>  scripts/qapi/parser.py                       | 11 ++++++++---
> >>  tests/qapi-schema/doc-duplicated-return.err  |  2 +-
> >>  tests/qapi-schema/doc-duplicated-return.json |  1 +
> >>  tests/qapi-schema/doc-duplicated-since.err   |  2 +-
> >>  tests/qapi-schema/doc-duplicated-since.json  |  1 +
> >>  tests/qapi-schema/doc-good.json              |  9 +++++++++
> >>  tests/qapi-schema/doc-invalid-return.err     |  2 +-
> >>  tests/qapi-schema/doc-invalid-return.json    |  1 +
> >>  9 files changed, 32 insertions(+), 12 deletions(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> >
> >> @@ -574,9 +577,11 @@ def end_comment(self) -> None:
> >>      def _match_at_name_colon(string: str) -> Optional[Match[str]]:
> >>          return re.match(r'@([^:]*): *', string)
> >>  
> >> -    @staticmethod
> >> -    def _match_section_tag(string: str) -> Optional[Match[str]]:
> >> -        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
> >> +    def _match_section_tag(self, string: str) -> Optional[Match[str]]:
> >> +        if not self._first_line_in_paragraph:
> >> +            return None
> >> +        return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *',
> >> +                        string)
> >
> > I guess I have a minor worry that we're silently ignoring
> > these tags when there's no blank line. Could result in
> > docs silently rendering in the wrong way if (when) someone
> > forgets the blank line.
> 
> True.
> 
> We could make it an error.  Can flag occurences in the middle of a
> free-form paragraph that happen to be at the beginning of a line, but
> that seems unlikely.
> 
> Of course, rST is full of similar syntactic traps anyway.  To quote
> Paolo, "I find it to be the Perl of ASCII-based markups."

Yes, probably not worth the hassle.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|