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`.
+    """
     assert not typ.variants
     ret = mcgen('''
     %(c_name)s param = {
-- 
2.26.2

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

> +    """
>      assert not typ.variants
>      ret = mcgen('''
>      %(c_name)s param = {
> -- 
> 2.26.2
> 

-- 
Eduardo

On 9/23/20 10:48 AM, Eduardo Habkost wrote:
> 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?
> - 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.
> 

Good questions for Markus.

(By the way, Markus: I do intend to remove the "missing-docstring" 
warning from the exceptions, and at such time we can have a party 
writing docstrings for everything.

I intend to devote an entire series to doing this during the release 
window.)

--js

>> +    """
>>       assert not typ.variants
>>       ret = mcgen('''
>>       %(c_name)s param = {
>> -- 
>> 2.26.2
>>
> 

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

On 9/25/20 8:19 AM, Markus Armbruster wrote:
> What about:
> 
>      Generate a QAPI struct variable holding the event parameters,
>      initialized with the function's arguments.

Line length and style-guide limitations; docstrings need a one-liner 
summary.

(Consistency is the hobgoblin, blah blah blah.)

I am writing:

     """
     Generate a QAPI struct variable with an initializer.

     The QAPI struct describes the event parameters, and the initializer
     references the function arguments defined in `gen_event_send`.
     """

John Snow <jsnow@redhat.com> writes:

> On 9/25/20 8:19 AM, Markus Armbruster wrote:
>> What about:
>>      Generate a QAPI struct variable holding the event parameters,
>>      initialized with the function's arguments.
>
> Line length and style-guide limitations; docstrings need a one-liner
> summary.

They do!

> (Consistency is the hobgoblin, blah blah blah.)
>
> I am writing:
>
>     """
>     Generate a QAPI struct variable with an initializer.
>
>     The QAPI struct describes the event parameters, and the initializer
>     references the function arguments defined in `gen_event_send`.
>     """

Better.

My second try:

      Generate a struct variable holding the event parameters.

      Initialize it with the function arguments defined in in
      `gen_event_send`.

On 9/28/20 7:49 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 9/25/20 8:19 AM, Markus Armbruster wrote:
>>> What about:
>>>       Generate a QAPI struct variable holding the event parameters,
>>>       initialized with the function's arguments.
>>
>> Line length and style-guide limitations; docstrings need a one-liner
>> summary.
> 
> They do!
> 
>> (Consistency is the hobgoblin, blah blah blah.)
>>
>> I am writing:
>>
>>      """
>>      Generate a QAPI struct variable with an initializer.
>>
>>      The QAPI struct describes the event parameters, and the initializer
>>      references the function arguments defined in `gen_event_send`.
>>      """
> 
> Better.
> 
> My second try:
> 
>        Generate a struct variable holding the event parameters.
> 
>        Initialize it with the function arguments defined in in
>        `gen_event_send`.
> 

You're the boss!

--js

On Tue, Sep 22, 2020 at 05:00:41PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>