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

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4008 bytes --]

On Thu, May 16, 2024, 1:58 AM Markus Armbruster <armbru@redhat.com> wrote:

> 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 :)
>

Well. It wasn't intentional, so it wasn't a lie... it was just wrong :)


> 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.
>

Hmm... Did I really change its meaning? I guess it's debatable what "where
it begins" means. Does the tagless section start...

## <-- Here?
# Hello! <-- Or here?
##

I assert the *section* starts wherever the first line of text it contains
starts. Nothing else makes any sense.

There is value in recording where the doc block starts, but that's not a
task for the *section* info.

I don't think I understand your feedback.


> 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)
>
>

[-- Attachment #2: Type: text/html, Size: 6288 bytes --]