[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section |
Date: |
Mon, 27 May 2024 13:58:09 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
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)
>>
>>
[PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition, John Snow, 2024/05/14
[PATCH 15/20] qapi: remove developer factoring comments from QAPI doc blocks, John Snow, 2024/05/14
[PATCH 12/20] qapi/source: allow multi-line QAPISourceInfo advancing, John Snow, 2024/05/14
[PATCH 16/20] qapi: rewrite StatsFilter comment, John Snow, 2024/05/14
[PATCH 14/20] qapi: fix non-compliant JSON examples, John Snow, 2024/05/14
[PATCH 17/20] qapi: rewrite BlockExportOptions doc block, John Snow, 2024/05/14
[PATCH 18/20] qapi: ensure all errors sections are uniformly typset, John Snow, 2024/05/14
[PATCH 19/20] qapi: convert "Note" sections to plain rST, John Snow, 2024/05/14
[PATCH 20/20] qapi: convert "Example" sections to rST, John Snow, 2024/05/14
Re: [PATCH 00/20] qapi: new sphinx qapi domain pre-requisites, Stefan Hajnoczi, 2024/05/16