On Mon, May 27, 2024 at 7:58 AM Markus Armbruster <arm...@redhat.com> wrote:
> 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.
>
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)
> >>
> >>
>
>
