[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [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

Attachment: signature.asc
Description: This is a digitally signed message part.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]