[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 18/38] qapi/events.py: Move comments into docstrings
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 18/38] qapi/events.py: Move comments into docstrings |
Date: |
Fri, 25 Sep 2020 14:19:52 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Sep 22, 2020 at 05:00:41PM -0400, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/events.py | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 00a9513c16..e859fd7a52 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -52,8 +52,10 @@ def gen_event_send_decl(name: str,
>> proto=build_event_send_proto(name, arg_type, boxed))
>>
>>
>> -# Declare and initialize an object 'qapi' using parameters from
>> build_params()
>> def gen_param_var(typ: QAPISchemaObjectType) -> str:
>> + """
>> + Declare and initialize a qapi object, using parameters from
>> `build_params`.
>
> The mention of "object 'qapi'" is gone, and this seems correct
> because there's no object called 'qapi' anywhere in this
> function. So:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> Comments/questions for follow up patches, because it's beyond the
> scope of this series:
>
> - Was the doc string supposed to say "an object 'param'" instead
> of "an object 'qapi'", as that's the name of the variable it
> declares?
Checking history... yes. The variable was renamed from @qapi to @param
during review.
> - The "using parameters from build_params()" part was confusing to
> me. I thought it meant gen_param_var() would call build_params().
> I took a while to notice it actually meant "using the C
> function parameters that were previously declared using
> build_params() at build_event_send_proto()". I don't know
> how we could make it clearer.
What about:
Generate a QAPI struct variable holding the event parameters,
initialized with the function's arguments.
>> + """
>> assert not typ.variants
>> ret = mcgen('''
>> %(c_name)s param = {
>> --
>> 2.26.2
>>
- Re: [PATCH v2 04/38] qapi: Prefer explicit relative imports, (continued)
Re: [PATCH v2 18/38] qapi/events.py: Move comments into docstrings, Cleber Rosa, 2020/09/23
[PATCH v2 13/38] qapi/common.py: add type hint annotations, John Snow, 2020/09/22
Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations, Cleber Rosa, 2020/09/23
[PATCH v2 14/38] qapi/common.py: Convert comments into docstrings, and elaborate, John Snow, 2020/09/22