John Snow <js...@redhat.com> writes:
> This is being done primarily to ensure consistency between the source
> documents and the final, rendered HTML output. Because
> member/feature/returns sections will always appear in a visually grouped
> element in the HTML output, prohibiting free paragraphs between those
> sections ensures ordering consistency between source and the final
> render.
>
> Additionally, prohibiting such "middle" text paragraphs allows us to
> classify all plain text sections as either "intro" or "detail"
> sections, because these sections must either appear before structured
> elements ("intro") or afterwards ("detail").
>
> This keeps the inlining algorithm simpler with fewer "splice" points
> when inlining multiple documentation blocks.
Mention the two "middle" paragraphs you have to eliminate in this patch?
>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> qapi/net.json | 4 ++--
> qapi/qom.json | 4 ++--
> scripts/qapi/parser.py | 16 ++++++++++++++++
> tests/qapi-schema/doc-good.json | 4 ++--
> tests/qapi-schema/doc-good.out | 4 ++--
> tests/qapi-schema/doc-good.txt | 8 ++++----
> 6 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 2739a2f4233..49bc7de64e9 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -655,13 +655,13 @@
> # this to zero disables this function. This member is mutually
> # exclusive with @reconnect. (default: 0) (Since: 9.2)
> #
> -# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> -#
> # Features:
> #
> # @deprecated: Member @reconnect is deprecated. Use @reconnect-ms
> # instead.
> #
> +# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> +#
> # Since: 7.2
> ##
> { 'struct': 'NetdevStreamOptions',
The text moved applies to member @addr. You're moving it even farther
away from @addr. Move it into @addr instead? Could be done as a
separate cleanup patch to keep this one as simple as possible; matter of
taste.
The same text is in NetdevDgramOptions below, where it applies to both
@remote and @local. It just happens to follow @remote and @local
immediately, because there are no other members and no features. Hmm.
Ideally, we'd have a way to put such notes next to the stuff they apply
to without having to rely on happy accidents like "no features".
Alternatively, have a way to link stuff and note. Footnotes? Food for
thought, not demand.
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d0..11277d1f84c 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -195,12 +195,12 @@
> #
> # @typename: the type name of an object
> #
> +# Returns: a list of ObjectPropertyInfo describing object properties
> +#
> # .. note:: Objects can create properties at runtime, for example to
> # describe links between different devices and/or objects. These
> # properties are not included in the output of this command.
> #
> -# Returns: a list of ObjectPropertyInfo describing object properties
> -#
> # Since: 2.12
> ##
This move is fine. Placing notes at the end is more common already.
> { 'command': 'qom-list-properties',
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b2f77ffdd7a..c5d2b950a82 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
> self.accept(False)
> line = self.get_doc_line()
> have_tagged = False
> + no_more_tags = False
> +
> + def _tag_check(what: str) -> None:
> + if what in ('TODO', 'Since'):
> + return
> +
> + if no_more_tags:
> + raise QAPIParseError(
> + self,
> + f"{what!r} section cannot appear after free "
> + "paragraphs that follow other tagged sections. "
> + "Move this section upwards with the preceding "
> + "tagged sections."
> + )
Why !r conversion?
Error messages should be a single, short phrase, no punctuation at the
end. Sometimes a helpful hint is desirable. Here's one in expr.py:
raise QAPISemError(
info,
"%s has unknown key%s %s\nValid keys are %s."
% (source, 's' if len(unknown) > 1 else '',
pprint(unknown), pprint(allowed)))
Needs a negative test case.
Aside: we should probably convert most string interpolation to f-strings
en masse at some point.
>
> while line is not None:
> # Blank lines
> @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
> if doc.features:
> raise QAPIParseError(
> self, "duplicated 'Features:' line")
> + _tag_check("Features")
> self.accept(False)
> line = self.get_doc_line()
> while line == '':
> @@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
> )
> raise QAPIParseError(self, emsg)
>
> + _tag_check(match.group(1))
> doc.new_tagged_section(
> self.info,
> QAPIDoc.Kind.from_string(match.group(1))
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index f64bf38d854..14b2091b08f 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -157,12 +157,12 @@
> # @cmd-feat1: a feature
> # @cmd-feat2: another feature
> #
> -# .. note:: @arg3 is undocumented
> -#
> # Returns: @Object
> #
> # Errors: some
> #
> +# .. note:: @arg3 is undocumented
> +#
This used to be right next to @arg1 and arg2 (commit 80d1f2e4a5d) until
commit 79598c8a634 added features in between. This patch adds more
stuff there. Right next is clearly the best spot, but this is just a
test, so it doesn't really matter. Related: NetdevDgramOptions' note
discussed above.
> # TODO: frobnicate
> #
> # .. admonition:: Notes
[...]
