Libguestfs - Jul 2016 - Re: [PATCH v2 2/3] mllib: Use L"..." and S '...' for long and short options.

On Monday, 18 July 2016 11:46:46 CEST Richard W.M. Jones
wrote:
> ---
Note that this changes the way -foo options are handled: this basically
makes them as --foo, but still working as -foo because getopt_long_only
is used.  IMHO either add a new M".." ([M]edium or [T]runcated or
[D]ash or ...), or turn S to get a string instead.
> -  let validate_key key > -    if String.length key == 0 || key ==
"-" || key == "--"
> -       || key.[0] != '-' then
> -      invalid_arg (sprintf "invalid option key: '%s'"
key)
> +  let validate_key = function
> +    | L"" -> invalid_arg "Getopt spec: invalid empty
long option"
> +    | L"help" -> invalid_arg "Getopt spec: should not
have L\"help\""
Theoretically both Arg and the current Getopt allow applications to
provide an own handler for --help, instead of the built-in one.
> +    | L s when String.contains s '_' ->
> +       invalid_arg (sprintf "Getopt spec: L%S should not contain
'_'"
> +                            s)
Why this limitation?
> -  let t > -    {
> -      specs = [];  (* Set it later, with own options, and sorted.  *)
> -      anon_fun = anon_fun;
> -      usage_msg = usage_msg;
> -    } in
> -
> -  let specs = specs @ [
> -    [ "--short-options" ], Unit (display_short_options t),
hidden_option_description;
> -    [ "--long-options" ], Unit (display_long_options t),
hidden_option_description;
> -  ] in
> -
> -  (* Decide whether the help option can be added, and which switches use. 
*)
> -  let has_dash_help = ref false in
> -  let has_dash_dash_help = ref false in
> -  List.iter (
> -    fun (keys, _, _) ->
> -      if not (!has_dash_help) then
> -        has_dash_help := List.mem "-help" keys;
> -      if not (!has_dash_dash_help) then
> -        has_dash_dash_help := List.mem "--help" keys;
> -  ) specs;
> -  let help_keys = [] @
> -    (if !has_dash_help then [] else [ "-help" ]) @
> -    (if !has_dash_dash_help then [] else [ "--help" ]) in
> -  let specs = specs @
> -    (if help_keys <> [] then [ help_keys, Unit (show_help t),
s_"Display brief help"; ] else []) in
> -
> -  (* Sort the specs, and set them in the handle.  *)
> +  (* Sort the specs.  *)
>    let specs = List.map (
>      fun (keys, action, doc) ->
>        List.hd (List.sort compare_command_line_args keys), (keys, action,
doc)
> @@ -194,14 +179,26 @@ let create specs ?anon_fun usage_msg >      let
cmp (arg1, _) (arg2, _) = compare_command_line_args arg1 arg2 in
>      List.sort cmp specs in
>    let specs = List.map snd specs in
> -  t.specs <- specs;
>  
> +  let t = {
> +    specs = specs;
> +    anon_fun = anon_fun;
> +    usage_msg = usage_msg;
> +  } in
> +  let added_options = [
> +    [ L"short-options" ], Unit (display_short_options t),
> +                                         hidden_option_description;
> +    [ L"long-options" ], Unit (display_long_options t),
> +                                         hidden_option_description;
> +    [ L"help" ], Unit (show_help t),     s_"Display brief
help";
> +  ] in
> +  t.specs <- added_options @ specs;
IMHO it'd be better to sort the specs at this point, like done before;
otherwise, --help (and potentially any non-hidden built-in option added
here) will be shown only at the end of the other specs.

Thanks,
-- 
Pino Toscano

On Mon, Jul 18, 2016 at 03:21:31PM +0200, Pino Toscano
wrote:
> On Monday, 18 July 2016 11:46:46 CEST Richard W.M. Jones wrote:
> > ---
> 
> Note that this changes the way -foo options are handled: this basically
> makes them as --foo, but still working as -foo because getopt_long_only
> is used.  IMHO either add a new M".." ([M]edium or [T]runcated or
> [D]ash or ...), or turn S to get a string instead.
Do you mean only for the 'virt-v2v --help' output?  I don't think
there is any other place in the code where the M option would be
handled differently from the L option.
> > -  let validate_key key > > -    if String.length key == 0 ||
key == "-" || key == "--"
> > -       || key.[0] != '-' then
> > -      invalid_arg (sprintf "invalid option key:
'%s'" key)
> > +  let validate_key = function
> > +    | L"" -> invalid_arg "Getopt spec: invalid
empty long option"
> > +    | L"help" -> invalid_arg "Getopt spec: should
not have L\"help\""
> 
> Theoretically both Arg and the current Getopt allow applications to
> provide an own handler for --help, instead of the built-in one.
Sure, but I don't think that's a good idea :-)
> > +    | L s when String.contains s '_' ->
> > +       invalid_arg (sprintf "Getopt spec: L%S should not contain
'_'"
> > +                            s)
> 
> Why this limitation?
I guess we don't need this limitation.  It's there to catch accidental
underscores.  If we actually have to use underscore parameters then we
can remove this later.
> IMHO it'd be better to sort the specs at this point, like done before;
> otherwise, --help (and potentially any non-hidden built-in option added
> here) will be shown only at the end of the other specs.
At the beginning of the list, and it was deliberate.  However it's
just a matter of preference, and the options could be sorted later.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Monday, 18 July 2016 14:43:03 CEST Richard W.M. Jones
wrote:
> > > -  let validate_key key > > > -    if String.length key
== 0 || key == "-" || key == "--"
> > > -       || key.[0] != '-' then
> > > -      invalid_arg (sprintf "invalid option key:
'%s'" key)
> > > +  let validate_key = function
> > > +    | L"" -> invalid_arg "Getopt spec: invalid
empty long option"
> > > +    | L"help" -> invalid_arg "Getopt spec:
should not have L\"help\""
> > 
> > Theoretically both Arg and the current Getopt allow applications to
> > provide an own handler for --help, instead of the built-in one.
> 
> Sure, but I don't think that's a good idea :-)
Well, I could have avoided that when proposing the final Getopt patch...

Also, there should be the same checks for M"something" of the
L"something" ones.
> > IMHO it'd be better to sort the specs at this point, like done
before;
> > otherwise, --help (and potentially any non-hidden built-in option
added
> > here) will be shown only at the end of the other specs.
> 
> At the beginning of the list, and it was deliberate.  However it's
> just a matter of preference, and the options could be sorted later.
Well I'd prefer it sorted, since it won't create arbitrary lines of
--help with different order, which look a bit odd.

Also, making everything ordered was basically the reason for the helper
type t, and the mutable state of its specs field.

-- 
Pino Toscano