diff mbox series

[v2,18/38] qapi/events.py: Move comments into docstrings

Message ID 20200922210101.4081073-19-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/events.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Eduardo Habkost Sept. 23, 2020, 2:48 p.m. UTC | #1
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
John Snow Sept. 23, 2020, 6:21 p.m. UTC | #2
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

>>

>
Cleber Rosa Sept. 23, 2020, 8:19 p.m. UTC | #3
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>
Markus Armbruster Sept. 25, 2020, 12:19 p.m. UTC | #4
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
>>
John Snow Sept. 25, 2020, 3:55 p.m. UTC | #5
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`.
     """
Markus Armbruster Sept. 28, 2020, 11:49 a.m. UTC | #6
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`.
John Snow Sept. 28, 2020, 3:04 p.m. UTC | #7
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
diff mbox series

Patch

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