| Message ID | 20240514215740.940155-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: new sphinx qapi domain pre-requisites | expand |
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 :) 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. 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)
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. > 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) > >
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) >> >>
On Mon, May 27, 2024 at 7:58 AM Markus Armbruster <armbru@redhat.com> wrote: > 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) > >> > >> > >
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' + 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' return # start new section section = self.Section(info)
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(-)