| Message ID | 20240216145841.2099240-12-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Doc comment parsing & doc generation work | expand |
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
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."
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
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' }
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(-)