John Snow <js...@redhat.com> writes: > On Thu, May 16, 2024, 1:58 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@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 <js...@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) >> >>