dbuenzli / cmdliner

Declarative definition of command line interfaces for OCaml
http://erratique.ch/software/cmdliner
ISC License
296 stars 56 forks source link

Add support for flag negation e.g. --flag/--no-flag #164

Closed mjambon closed 2 years ago

mjambon commented 2 years ago

I'm translating Python code that provides --foo to provide the value true and --no-foo to provide false. In an effort to maximize backward compatibility, I would like to provide the same functionality to my CLI written in OCaml. Here's how I define an option as a flag:

let o_autofix =
  let info =
    Arg.info [ "a"; "autofix" ]
      ~doc:"Apply autofix patches. WARNING: data loss can occur with this \
            flag. Make sure your files are stored in a version control \
            system. Note that this mode is experimental and not guaranteed \
            to function properly."
  in
  Arg.value (Arg.flag info)

The command-line help shows this:

       -a, --autofix
           Apply autofix patches. WARNING: data loss can occur with this flag.
           Make sure your files are stored in a version control system. Note
           that this mode is experimental and not guaranteed to function
           properly.

I would like --no-autofix to be offered. What's a good way to achieve that?

mjambon commented 2 years ago

Note that --no-foo is useless if the default value of foo is known to always be false. It's useful however when the default value is false or when the user doesn't want to rely on the default.

mjambon commented 2 years ago

I'm considering the following approach. It works but adds complexity and reduces readability for the developer. It also places the --no-foo option far away from --foo on the help page due to sorting in alphabetic order. Here's the code:

(* CLI_common.ml *)

let negatable_flag ?short_option long_option ~doc =
  let options =
    match short_option with
    | None -> [long_option]
    | Some x -> [String.make 1 x; long_option]
  in
  let neg_option = "no-" ^ long_option in
  let neg_options = [neg_option] in
  let info = Arg.info options ~doc in
  let neg_info =
    Arg.info neg_options ~doc:(sprintf "negates --%s" long_option)
  in
  let arg = Arg.value (Arg.flag info) in
  let neg_arg = Arg.value (Arg.flag neg_info) in
  let select_flag_value value neg_value =
    if value && neg_value then
      failwith (
        sprintf "command-line confusion: both %s and %s are specified"
          long_option neg_option
      )
    else if value then
      true
    else if neg_value then
      false
    else
      false
  in
  arg, neg_arg, select_flag_value

It's used as follows:

let o_autofix, o_no_autofix, select_autofix_value =
  CLI_common.negatable_flag ~short_option:'a' "autofix"
    ~doc:"Apply autofix patches. WARNING: data loss can occur with this flag. \
          Make sure your files are stored in a version control system. Note \
          that this mode is experimental and not guaranteed to function \
          properly."

...
let cmdline_term run =
  let combine autofix no_autofix baseline_commit metrics =
    (* Ensure at most one of --autofix and --no-autofix was given *)
    let autofix = select_autofix_value autofix no_autofix in
    run { autofix; baseline_commit; metrics }
  in
  Term.(const combine $ o_autofix $ o_no_autofix $ o_baseline_commit $ o_metrics)

The help page looks like this:

OPTIONS
       -a, --autofix
           Apply autofix patches. WARNING: data loss can occur with this flag.
           Make sure your files are stored in a version control system. Note
           that this mode is experimental and not guaranteed to function
           properly.

       --baseline_commit=VAL (absent SEMGREP_BASELINE_COMMIT env)
           Only show results that are not found in this commit hash. Aborts
           run if not currently in a git directory, there are unstaged
           changes, or given baseline hash doesn't exist

       --metrics=VAL (absent=auto or SEMGREP_SEND_METRICS env)
           Configures how usage metrics are sent to the Semgrep server. If
           'auto', metrics are sent whenever the --config value pulls from the
           Semgrep server. If 'on', metrics are always sent. If 'off', metrics
           are disabled altogether and not sent. If absent, the
           SEMGREP_SEND_METRICS environment variable value will be used. If no
           environment variable, defaults to 'auto'.

       --no-autofix
           negates --autofix

Instead, I would prefer:

       -a, --autofix / --no-autofix
           Apply autofix patches. WARNING: data loss can occur with this flag.
           Make sure your files are stored in a version control system. Note
           that this mode is experimental and not guaranteed to function
           properly.

This could be provided via an optional argument neg:

let info = Arg.info ["a"; "autofix"] ~neg:["no-autofix"] ~doc 

The neg argument would not make sense in the case of arguments other than flags. It would be checked at runtime like the list of short/long option names.

dbuenzli commented 2 years ago

I don't think --flag/--no-flag is a very good pattern so I'm not going to provide support for it.

I'm considering the following approach.

This can be made shorter using Arg.vflag for example:

open Cmdliner

let revolt really = if really then print_endline "Revolt!"

let really =
  let really = true, Arg.info ["really"] ~doc:"Really" in
  let not_really = false, Arg.info ["not-really"] ~doc:"Not really" in
  Arg.(value & vflag true [really; not_really])

let cmd = Cmd.v (Cmd.info "revolt") Term.(const revolt $ really)
let () = exit (Cmd.eval cmd)

This however won't solve this problem:

It also places the --no-foo option far away from --foo on the help page due to sorting in alphabetic order

for which I suggest to simply mention the negation in the doc string of each other in one way or another.

mjambon commented 2 years ago

Thank you! Arg.vflag will do nicely. Here's the updated code I'm planning on using, for posterity:

(* Turn "a" into "-a" and "abc" into "--abc" *)
let add_option_dashes option_names =
  List.map (fun s ->
    assert (s <> "");
    if String.length s = 1 then "-" ^ s
    else "--" ^ s
  ) option_names

(* Define a flag that can be negated e.g. --foo and --no-foo.
   It's not supported out-of-the-box by cmdliner but we want it for
   backward compatibility with the Python CLI.
   See https://github.com/dbuenzli/cmdliner/issues/164
*)
let negatable_flag ~options ~neg_options ~doc =
  let neg_doc =
    let options_str = add_option_dashes options |> String.concat "/" in
    sprintf "negates %s" options_str
  in
  let default = false in
  let enable = true, Arg.info options ~doc in
  let disable = false, Arg.info neg_options ~doc:neg_doc in
  Arg.value (Arg.vflag default [enable; disable])

and

let o_autofix =
  CLI_common.negatable_flag
    ~options:["a"; "autofix"]
    ~neg_options:["no-autofix"]
    ~doc:"Apply autofix patches. WARNING: data loss can occur with this flag. \
          Make sure your files are stored in a version control system. Note \
          that this mode is experimental and not guaranteed to function \
          properly."
dbuenzli commented 2 years ago

You didn't mention how your original lib handles multiple occurences of the flags. Arg.vflag will error. If you want "last flag takes over" you can use Arg.flag_all and Arg.last.