Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Instead of using the info object for the doc block as a whole, update
> the info pointer for each call to ensure_untagged_section when the
> existing section is otherwise empty. This way, Sphinx error information
> will match precisely to where the text actually starts.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 8cdd5334ec6..41b9319e5cb 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -662,8 +662,13 @@ def end(self) -> None:
>  
>      def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>          if self.all_sections and not self.all_sections[-1].tag:
> -            # extend current section
> -            self.all_sections[-1].text += '\n'

Before, we always append a newline.

> +            section = self.all_sections[-1]
> +            # Section is empty so far; update info to start *here*.
> +            if not section.text:
> +                section.info = info
> +            else:
> +                # extend current section
> +                self.all_sections[-1].text += '\n'

Afterwards, we append it only when the section already has some text.

The commit message claims the patch only adjusts section.info.  That's a
lie :)

I believe the change makes no difference because .end() strips leading
and trailing newline.

>              return
>          # start new section
>          section = self.Section(info)

You could fix the commit message, but I think backing out the
no-difference change is easier.  The appended patch works in my testing.

Next one.  Your patch changes the meaning of section.info.  Here's its
initialization:

    class Section:
        # pylint: disable=too-few-public-methods
        def __init__(self, info: QAPISourceInfo,
                     tag: Optional[str] = None):
--->        # section source info, i.e. where it begins
            self.info = info
            # section tag, if any ('Returns', '@name', ...)
            self.tag = tag
            # section text without tag
            self.text = ''

The comment is now wrong.  Calls for a thorough review of .info's uses.

The alternative to changing .info's meaning is to add another member
with the meaning you need.  Then we have to review .info's uses to find
out which ones to switch to the new one.

Left for later.


diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8cdd5334ec..abeae1ca77 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -663,7 +663,10 @@ def end(self) -> None:
     def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
         if self.all_sections and not self.all_sections[-1].tag:
             # extend current section
-            self.all_sections[-1].text += '\n'
+            section = self.all_sections[-1]
+            if not section.text:
+                section.info = info
+            section.text += '\n'
             return
         # start new section
         section = self.Section(info)