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