| Message ID | 20200922210101.4081073-19-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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 >> >
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>
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
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 = {
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/events.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)