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

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

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

This was before my vacation, and my memory is foggy, ...  I may have
gotten confused back then.  Let me have a fresh look now.

self.info gets initialized in Section.__init__() to whatever info it
gets passed.

Your patch makes .ensure_untagged_section() overwrite this Section.info
when it extends an untagged section that is still empty.  Hmmm.  I'd
prefer .info to remain constant after initialization.

I figure this overwrite can happen only when extenting the body section
QAPIDoc.__init__() creates.  In that case, it adjusts .info from
beginning of doc comment to first non-blank line.

Thoughts?

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