On Mon, Jan 11, 2016 at 05:26:55PM +0100, Andrea Bolognani wrote:
> On Mon, 2016-01-11 at 10:55 -0500, John Ferlan wrote:
> >
> > > The config option for the 'schedinfo' and 'change-media'
> > > commands, while it has a slightly different help text, also
> > > serves AFAICT the same purpose and as such should IMHO use the
> > > macro you just defined as well.
> >
> > BTW: I could do something similar as done for the _FILE w/r/t _helpstr
> > for any ".type = VSH_OT_BOOL," options... To avoid the multiple places
> > where I'd pass "N_("affect next boot"), I could change the macro to be:
> >
> >
> > #define VIRSH_COMMON_OPT_CONFIG(_helpstr) \
> > {.name = "config", \
> > .type = VSH_OT_BOOL, \
> > .help = _helpstr ? _helpstr : N_("affect next boot") \
> > } \
> >
> > and callers be either (NULL) or whatever helpstring - thoughts??
>
> I think that might be pushing it a little too far, because while
> browsing the code it would be easy to tell what
>
> VIRSH_COMMON_OPT_CONFIG(N_("modify/get persistent configuration"))
>
> means but
>
> VIRSH_COMMON_OPT_CONFIG(NULL)
>
> would be much more obscure.
>
> I'd rather have VIRSH_COMMON_OPT_CONFIG() require its argument and
> create extra macros on top of it, like I suggested doing in my
> comments on 08/14, eg.
>
> #define VIRSH_COMMON_OPT_DOMAIN_CONFIG \
> VIRSH_COMMON_OPT_CONFIG(N_("affect next boot"))
>
> because then, while reading the code using it, you would go
>
> «Oh, right, the usual -config options supported by pretty much
> all commands that act on domains!»
>
> Or at least that's what *I* would go :)
>
> Plus, in some cases, it might be difficult to decide what the
> default message should be... See again my comments on 08/14.
>
> Cheers.
Just a question, I know it's to late to change those patches, it's pushed now,
but why don't we unify the help string for all the commands? It does the same
thing for all commands, there is no reason to have different help string for
some commands. And I don't think, that it would break anything.
Pavel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list