Re: [PATCH 01/14] qapi: BlockExportRemoveMode: move comments to TODO

[Victor Toso]

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

signature.asc
Description: PGP signature