| Message ID | 20220324175015.232794-2-victortoso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some qapi examples and a TODO section | expand |
On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote: > > @hide and @soft are potential additions which fits the TODO section > perfectly. > > The main motivation is to avoid this whole block of comment entering > the wrong section in the python parser. > > Signed-off-by: Victor Toso <victortoso@redhat.com> > --- > qapi/block-export.json | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/qapi/block-export.json b/qapi/block-export.json > index f183522d0d..1e34927f85 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json > @@ -219,13 +219,13 @@ > # > # @hard: Drop all connections immediately and remove export. > # > -# Potential additional modes to be added in the future: > +# TODO: Potential additional modes to be added in the future: > # > -# hide: Just hide export from new clients, leave existing connections as is. > -# Remove export after all clients are disconnected. > +# hide: Just hide export from new clients, leave existing connections as is. > +# Remove export after all clients are disconnected. > # > -# soft: Hide export from new clients, answer with ESHUTDOWN for all further > -# requests from existing clients. > +# soft: Hide export from new clients, answer with ESHUTDOWN for all further > +# requests from existing clients. > # > # Since: 2.12 > ## > -- > 2.35.1 > Does this help with something in particular? (Got an example for me?) --js
Victor Toso <victortoso@redhat.com> writes: > @hide and @soft are potential additions which fits the TODO section > perfectly. > > The main motivation is to avoid this whole block of comment entering > the wrong section in the python parser. > > Signed-off-by: Victor Toso <victortoso@redhat.com> > --- > qapi/block-export.json | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/qapi/block-export.json b/qapi/block-export.json > index f183522d0d..1e34927f85 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json > @@ -219,13 +219,13 @@ > # > # @hard: Drop all connections immediately and remove export. > # > -# Potential additional modes to be added in the future: > +# TODO: Potential additional modes to be added in the future: > # > -# hide: Just hide export from new clients, leave existing connections as is. > -# Remove export after all clients are disconnected. > +# hide: Just hide export from new clients, leave existing connections as is. > +# Remove export after all clients are disconnected. > # > -# soft: Hide export from new clients, answer with ESHUTDOWN for all further > -# requests from existing clients. > +# soft: Hide export from new clients, answer with ESHUTDOWN for all further > +# requests from existing clients. > # > # Since: 2.12 > ## Reviewed-by: Markus Armbruster <armbru@redhat.com> Doc comments embed user documentation in the source code. The doc generator extracts it. TODOs are generally for developers. Should the doc generator suppress TODO sections?
On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote: > Victor Toso <victortoso@redhat.com> writes: > > > @hide and @soft are potential additions which fits the TODO section > > perfectly. > > > > The main motivation is to avoid this whole block of comment entering > > the wrong section in the python parser. > > > > Signed-off-by: Victor Toso <victortoso@redhat.com> > > --- > > qapi/block-export.json | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/qapi/block-export.json b/qapi/block-export.json > > index f183522d0d..1e34927f85 100644 > > --- a/qapi/block-export.json > > +++ b/qapi/block-export.json > > @@ -219,13 +219,13 @@ > > # > > # @hard: Drop all connections immediately and remove export. > > # > > -# Potential additional modes to be added in the future: > > +# TODO: Potential additional modes to be added in the future: > > # > > -# hide: Just hide export from new clients, leave existing connections > as is. > > -# Remove export after all clients are disconnected. > > +# hide: Just hide export from new clients, leave existing > connections as is. > > +# Remove export after all clients are disconnected. > > # > > -# soft: Hide export from new clients, answer with ESHUTDOWN for all > further > > -# requests from existing clients. > > +# soft: Hide export from new clients, answer with ESHUTDOWN for > all further > > +# requests from existing clients. > > # > > # Since: 2.12 > > ## > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Doc comments embed user documentation in the source code. The doc > generator extracts it. > > TODOs are generally for developers. Should the doc generator suppress > TODO sections? > Needs an audit to make sure we're using it consistently with that semantic, but broadly it's probably a good idea to squelch "internal" todos, yes. Things like "Watch out, were definitely gonna deprecate this soon probably maybe!" can stay outside of the TODO section. (Sometimes heads up are legitimate, even if most won't read them. the faithful and diligent will be rewarded with painless upgrades.) Anyway, if Markus is happy with this change, I am too, I was just curious to know if there were bigger cleanups to do here and what the impact was. Anyway: Reviewed-by: John Snow <jsnow@redhat.com> >
Hi, Many thanks for the quick review! On Thu, Mar 24, 2022 at 04:34:42PM -0400, John Snow wrote: > On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote: > > > > @hide and @soft are potential additions which fits the TODO section > > perfectly. > > > > The main motivation is to avoid this whole block of comment entering > > the wrong section in the python parser. > > > > Signed-off-by: Victor Toso <victortoso@redhat.com> > > --- > > qapi/block-export.json | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/qapi/block-export.json b/qapi/block-export.json > > index f183522d0d..1e34927f85 100644 > > --- a/qapi/block-export.json > > +++ b/qapi/block-export.json > > @@ -219,13 +219,13 @@ > > # > > # @hard: Drop all connections immediately and remove export. > > # > > -# Potential additional modes to be added in the future: > > +# TODO: Potential additional modes to be added in the future: > > # > > -# hide: Just hide export from new clients, leave existing connections as is. > > -# Remove export after all clients are disconnected. > > +# hide: Just hide export from new clients, leave existing connections as is. > > +# Remove export after all clients are disconnected. > > # > > -# soft: Hide export from new clients, answer with ESHUTDOWN for all further > > -# requests from existing clients. > > +# soft: Hide export from new clients, answer with ESHUTDOWN for all further > > +# requests from existing clients. > > # > > # Since: 2.12 > > ## > > -- > > 2.35.1 > > > > Does this help with something in particular? (Got an example for me?) I'm working on a Golang interface and I'm using the QAPI documentation to document the Go's types of the QAPI spec. For this kind of documentation, documentation related to future development can be excluded. This patch helps me filter it out :) Example: $ cd qemu/scripts && python >>> from qapi.schema import QAPISchema >>> schema = QAPISchema('../qapi/qapi-schema.json') # Without this patch, the 'Potential additional modes' doc is # under no specific named Section. >>> for s in schema._entity_dict['BlockExportRemoveMode'].doc.sections: ... pprint(vars(s)) ... {'_indent': 0, '_parser': <qapi.parser.QAPISchemaParser object at 0x7f4fcf854760>, 'name': None, 'text': 'Potential additional modes to be added in the future:\n' '\n' 'hide: Just hide export from new clients, leave existing connections ' 'as is.\n' 'Remove export after all clients are disconnected.\n' '\n' 'soft: Hide export from new clients, answer with ESHUTDOWN for all ' 'further\n' 'requests from existing clients.'} {'_indent': 7, '_parser': <qapi.parser.QAPISchemaParser object at 0x7f4fcf854760>, 'name': 'Since', 'text': '2.12'} # With this patch, we can filter out TODO section >>> pprint(schema._entity_dict['BlockExportRemoveMode'].doc.sections[0])) {'_indent': 6, '_parser': <qapi.parser.QAPISchemaParser object at 0x7f228d97e950>, 'name': 'TODO', 'text': 'Potential additional modes to be added in the future:\n' '\n' 'hide: Just hide export from new clients, leave existing connections ' 'as is.\n' 'Remove export after all clients are disconnected.\n' '\n' 'soft: Hide export from new clients, answer with ESHUTDOWN for all ' 'further\n' 'requests from existing clients.'} Cheers, Victor
Hi, On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote: > On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote: > > > Victor Toso <victortoso@redhat.com> writes: > > > > > @hide and @soft are potential additions which fits the TODO section > > > perfectly. > > > > > > The main motivation is to avoid this whole block of comment entering > > > the wrong section in the python parser. > > > > > > Signed-off-by: Victor Toso <victortoso@redhat.com> > > > --- > > > qapi/block-export.json | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/qapi/block-export.json b/qapi/block-export.json > > > index f183522d0d..1e34927f85 100644 > > > --- a/qapi/block-export.json > > > +++ b/qapi/block-export.json > > > @@ -219,13 +219,13 @@ > > > # > > > # @hard: Drop all connections immediately and remove export. > > > # > > > -# Potential additional modes to be added in the future: > > > +# TODO: Potential additional modes to be added in the future: > > > # > > > -# hide: Just hide export from new clients, leave existing connections > > as is. > > > -# Remove export after all clients are disconnected. > > > +# hide: Just hide export from new clients, leave existing > > connections as is. > > > +# Remove export after all clients are disconnected. > > > # > > > -# soft: Hide export from new clients, answer with ESHUTDOWN for all > > further > > > -# requests from existing clients. > > > +# soft: Hide export from new clients, answer with ESHUTDOWN for > > all further > > > +# requests from existing clients. > > > # > > > # Since: 2.12 > > > ## > > > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks, > > Doc comments embed user documentation in the source code. The doc > > generator extracts it. > > > > TODOs are generally for developers. Should the doc generator suppress > > TODO sections? > > Needs an audit to make sure we're using it consistently with > that semantic, but broadly it's probably a good idea to squelch > "internal" todos, yes. > > Things like "Watch out, were definitely gonna deprecate this > soon probably maybe!" can stay outside of the TODO section. > (Sometimes heads up are legitimate, even if most won't read > them. the faithful and diligent will be rewarded with painless > upgrades.) There are 5 TODO sections in QAPI (including this patch): qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future: qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP) qapi/machine.json:913:# TODO: Better documentation; currently there is none. qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its I think their usage is a bit broad but helpful. > Anyway, if Markus is happy with this change, I am too, I was > just curious to know if there were bigger cleanups to do here > and what the impact was. I'll let you know if I find more :) > Anyway: > > Reviewed-by: John Snow <jsnow@redhat.com> Thanks! I'll send a v2 later with all suggestions. Cheers, Victor
Victor Toso <victortoso@redhat.com> writes: > Hi, > > On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote: >> On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote: [...] >> > Doc comments embed user documentation in the source code. The doc >> > generator extracts it. >> > >> > TODOs are generally for developers. Should the doc generator suppress >> > TODO sections? >> >> Needs an audit to make sure we're using it consistently with >> that semantic, but broadly it's probably a good idea to squelch >> "internal" todos, yes. >> >> Things like "Watch out, were definitely gonna deprecate this >> soon probably maybe!" can stay outside of the TODO section. >> (Sometimes heads up are legitimate, even if most won't read >> them. the faithful and diligent will be rewarded with painless >> upgrades.) This is "future directions", not quite the same as "TODO". Would a section tag "Future directions" make sense? > There are 5 TODO sections in QAPI (including this patch): Let me try to sort them into "TODO" and "future directions" buckets. The former are of interest for developers only, and thus should be elided from documentation meant for users. > qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future: Do we believe our thoughts on evolving of this enum are relevant for users of the affected QMP commands (nbd-server-remove and block-export-del)? If yes, it's "future directions". > qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP) As phrased, this is only useful for developers, and even for them, it's rather terse. If we add introspection to QGA, we'll want to add a @success-response member. So, if we intend to add introspection to QGA, *and* we think current users of (QMP-only) introspection need to know about a future addition of @success-response, then this should be rephrased as "future directions". I doubt it. > qapi/machine.json:913:# TODO: Better documentation; currently there is none. Clearly TODO. > qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make Clearly TODO. Note that this one is *not* in a doc comment, and does *not* appear in generated documentation. Once we have concrete plans on how to address the TODO, these plans may motivate "future directions", namely if they involve user-visible change users need to know about in advance. > qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its Likewise. I think this shows that we have a few comments just for developers in the middle of user documentation. We could simply keep these outside doc comments, like the TODO in qapi/migration.json. This can occasionally be awkward. For instance, TODO @success-response is right where @success-response ought to be. Moving it outside the doc comment would lose that. Not the end of the world, just awkward. If this annoys us enough, we could provide means to let us have elide parts of doc comments from generated docs. The simplest one is probably eliding certain sections, say the TODO sections. Thoughts? [...]
On Mon, Mar 28, 2022 at 3:46 AM Markus Armbruster <armbru@redhat.com> wrote: > > Victor Toso <victortoso@redhat.com> writes: > > > Hi, > > > > On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote: > >> On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote: > > [...] > > >> > Doc comments embed user documentation in the source code. The doc > >> > generator extracts it. > >> > > >> > TODOs are generally for developers. Should the doc generator suppress > >> > TODO sections? > >> > >> Needs an audit to make sure we're using it consistently with > >> that semantic, but broadly it's probably a good idea to squelch > >> "internal" todos, yes. > >> > >> Things like "Watch out, were definitely gonna deprecate this > >> soon probably maybe!" can stay outside of the TODO section. > >> (Sometimes heads up are legitimate, even if most won't read > >> them. the faithful and diligent will be rewarded with painless > >> upgrades.) > > This is "future directions", not quite the same as "TODO". > > Would a section tag "Future directions" make sense? > If we're leaving these kinds of warnings already, sure. I was just speaking generically. I didn't actually audit all of our docs to see what types of notes we've left so far. > > There are 5 TODO sections in QAPI (including this patch): > > Let me try to sort them into "TODO" and "future directions" buckets. > The former are of interest for developers only, and thus should be > elided from documentation meant for users. > Oh, but it looks like *you* did the audit ;) > > qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future: > > Do we believe our thoughts on evolving of this enum are relevant for > users of the affected QMP commands (nbd-server-remove and > block-export-del)? > > If yes, it's "future directions". > Presumably this evolves compatibly like QMP always does, and doesn't necessarily warrant a special note. "future directions" feels reasonable. > > qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP) > > As phrased, this is only useful for developers, and even for them, it's > rather terse. > > If we add introspection to QGA, we'll want to add a @success-response > member. > > So, if we intend to add introspection to QGA, *and* we think current > users of (QMP-only) introspection need to know about a future addition > of @success-response, then this should be rephrased as "future > directions". > > I doubt it. > I'll defer to your judgment on this one. The last time I tried to bark into QGA I got more than I bargained for. :') > > qapi/machine.json:913:# TODO: Better documentation; currently there is none. > > Clearly TODO. > Yuh > > qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make > > Clearly TODO. Note that this one is *not* in a doc comment, and does > *not* appear in generated documentation. > > Once we have concrete plans on how to address the TODO, these plans may > motivate "future directions", namely if they involve user-visible change > users need to know about in advance. > > > qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its > > Likewise. > > I think this shows that we have a few comments just for developers in > the middle of user documentation. > > We could simply keep these outside doc comments, like the TODO in > qapi/migration.json. > > This can occasionally be awkward. For instance, TODO @success-response > is right where @success-response ought to be. Moving it outside the doc > comment would lose that. Not the end of the world, just awkward. > > If this annoys us enough, we could provide means to let us have elide > parts of doc comments from generated docs. The simplest one is probably > eliding certain sections, say the TODO sections. > > Thoughts? > I'm fine with it, maybe pending the spelling of such a section. Keeping the docs clean and useful on generation sounds like a noble endeavor. I don't think it's urgent, but I'm willing to let Victor tell me which pieces are annoying and getting in his way. (I still want to redo our docstring handling, but it wound up being a bigger project than I could bite off in a week or two. It's extremely high on my list of things to do, but I am already in debt to you on the typing project I wanted to engage on, so ... I will sit on my hands for right now.)
diff --git a/qapi/block-export.json b/qapi/block-export.json index f183522d0d..1e34927f85 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -219,13 +219,13 @@ # # @hard: Drop all connections immediately and remove export. # -# Potential additional modes to be added in the future: +# TODO: Potential additional modes to be added in the future: # -# hide: Just hide export from new clients, leave existing connections as is. -# Remove export after all clients are disconnected. +# hide: Just hide export from new clients, leave existing connections as is. +# Remove export after all clients are disconnected. # -# soft: Hide export from new clients, answer with ESHUTDOWN for all further -# requests from existing clients. +# soft: Hide export from new clients, answer with ESHUTDOWN for all further +# requests from existing clients. # # Since: 2.12 ##
@hide and @soft are potential additions which fits the TODO section perfectly. The main motivation is to avoid this whole block of comment entering the wrong section in the python parser. Signed-off-by: Victor Toso <victortoso@redhat.com> --- qapi/block-export.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)