All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org, "Zhiyi Guo" <zhguo@redhat.com>,
	"Daniel P . Berrangé" <berrange@redhat.com>,
	"Leonardo Bras Soares Passos" <lsoaresp@redhat.com>,
	"Fabiano Rosas" <farosas@suse.de>,
	"Juan Quintela" <quintela@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Chensheng Dong" <chdong@redhat.com>
Subject: Re: [PATCH for-8.2 v2 1/2] qapi/migration: Deduplicate migration parameter field comments
Date: Fri, 04 Aug 2023 14:28:05 +0200	[thread overview]
Message-ID: <87jzub8016.fsf@pond.sub.org> (raw)
In-Reply-To: <20230803155344.11450-2-peterx@redhat.com> (Peter Xu's message of "Thu, 3 Aug 2023 11:53:43 -0400")

Peter Xu <peterx@redhat.com> writes:

> We used to have three objects that have always the same list of parameters

We have!

> and comments are always duplicated:
>
>   - @MigrationParameter
>   - @MigrationParameters
>   - @MigrateSetParameters
>
> Before we can deduplicate the code, it's fairly straightforward to
> deduplicate the comments first, so for each time we add a new migration
> parameter we don't need to copy the same paragraphs three times.

De-duplicating the code would be nice, but we haven't done so in years,
which suggests it's hard enough not to be worth the trouble.

De-duplicating the documentation is certainly easier.

Is that what you're trying to say?

Our discussion pros and cons that is happening in review of v1 should be
captured in the commit message, right here.

> Make the @MigrationParameter the major source of truth, while leaving the
> rest two to reference to it.

Any particular reason for picking this one?

> We do have a slight problem in the man/html pages generated, that for the
> latter two objects we'll get a list of Members but with all of them saying
> "Not documented":
>
>    Members
>        announce-initial: int (optional)
>               Not documented
>
>        announce-max: int (optional)
>               Not documented
>
>        announce-rounds: int (optional)
>               Not documented
>
>        [...]
>
> Even though we'll have a reference there telling the reader to jump over to
> read the @MigrationParameter sections instead, for example:
>
>    MigrationParameters (Object)
>
>        The object structure to represent a list of migration parameters.
>        The optional members aren't actually optional.  For detailed
>        explanation for each of the field, please refer to the documentation
>        of MigrationParameter.
>
> So hopefully that's not too bad.. and we can leave it for later to make it
> even better.

It's plenty bad, I'm afraid.  It comes out as a short paragraph "don't
look here, look there", followed by screenfuls claiming "not
documented."  Embarrassing.  Worse, *misleading*, because the short
paragraph is easy to miss.

Also discussed in review of v1.  Let's continue there, to avoid
splitting the thread.

> Signed-off-by: Peter Xu <peterx@redhat.com>



  reply	other threads:[~2023-08-04 12:29 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-03 15:53 [PATCH for-8.2 v2 0/2] migration: Add max-switchover-bandwidth parameter Peter Xu
2023-08-03 15:53 ` [PATCH for-8.2 v2 1/2] qapi/migration: Deduplicate migration parameter field comments Peter Xu
2023-08-04 12:28   ` Markus Armbruster [this message]
2023-08-04 13:59     ` Daniel P. Berrangé
2023-08-04 16:01       ` Peter Xu
2023-08-04 16:29         ` Daniel P. Berrangé
2023-08-04 16:46           ` Peter Xu
2023-08-04 16:48             ` Daniel P. Berrangé
2023-08-04 21:02               ` Peter Xu
2023-08-05  8:12                 ` Markus Armbruster
2023-08-06 15:49                   ` Peter Xu
2023-08-08 20:03                     ` Peter Xu
2023-08-14 22:24                       ` Peter Xu
2023-08-03 15:53 ` [PATCH for-8.2 v2 2/2] migration: Allow user to specify migration switchover bandwidth Peter Xu
2023-08-31 18:14   ` Joao Martins
2023-08-31 18:34     ` Peter Xu
2023-09-01  6:55   ` Wang, Lei
2023-09-01  8:37     ` Daniel P. Berrangé
2023-09-01 14:40       ` Peter Xu
2023-09-05 16:46       ` Peter Xu
2023-09-05 17:39         ` Daniel P. Berrangé
2023-09-06  2:27         ` Wang, Lei
2023-09-01 17:59   ` Joao Martins
2023-09-01 18:39     ` Joao Martins
2023-09-05 15:31       ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87jzub8016.fsf@pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=chdong@redhat.com \
    --cc=eblake@redhat.com \
    --cc=farosas@suse.de \
    --cc=lsoaresp@redhat.com \
    --cc=peterx@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=zhguo@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.