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.
but, we don't have the right info when we initialize the entire QAPIDoc object, because the section hasn't truly actually started yet, so I don't think I can actually achieve your preference.
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.
Yes, that's the intended effect and in practice, the only time it actually happens.
This patch is necessary for accurate error reporting info, otherwise we get off-by-ones (or more, maybe). I believe the problem actually affects the current generator too (I don't see why it wouldn't), but I didn't test that because I'm trying to replace it.
Thoughts?
I think this patch is fine?.
>> 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)
>>
>>