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

[John Snow]

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