From 45ac3e5a2b648a91ba8442cb597e2969fa9074ec Mon Sep 17 00:00:00 2001
From: Nathan Rebours <nathan.p.rebours@gmail.com>
Date: Thu, 1 Jul 2021 11:31:33 +0200
Subject: [PATCH] Add --no-auto-open to default command and improve config CLI
 args mgmt

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
---
 CHANGES.md       |  2 ++
 bin/bistro.ml    | 14 ++++++++------
 bin/check.ml     |  1 +
 bin/cli.ml       | 29 ++++++++++++++++++++++-------
 bin/cli.mli      | 14 ++++++++++----
 bin/distrib.ml   |  2 +-
 bin/distrib.mli  |  2 +-
 bin/lint.ml      |  2 +-
 bin/opam.ml      | 14 ++++----------
 bin/opam.mli     | 10 +++++-----
 bin/publish.ml   |  2 +-
 bin/publish.mli  |  4 ++--
 bin/undraft.ml   |  2 +-
 lib/config.ml    | 18 +++++++++++++-----
 lib/config.mli   | 30 ++++++++++++++++++++----------
 lib/delegate.ml  |  2 +-
 lib/delegate.mli |  2 +-
 17 files changed, 94 insertions(+), 56 deletions(-)

diff --git a/CHANGES.md b/CHANGES.md
index f1bb5f48..4ecfbcb3 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -2,6 +2,8 @@
 
 ### Added
 
+- Add `--no-auto-open` to the default command. It was previously only available for
+  `dune-release opam`. (#374, @NathanReb)
 - Add a `config create` subcommand to create a fresh configuration if you don't have one yet
   (#373, @NathanReb)
 - Add `--local-repo`, `--remote-repo` and `--opam-repo` options to the default command,
diff --git a/bin/bistro.ml b/bin/bistro.ml
index 439e2487..2ff02bf9 100644
--- a/bin/bistro.ml
+++ b/bin/bistro.ml
@@ -12,10 +12,11 @@ let ( >! ) x f = match x with Ok 0 -> f () | _ -> x
 let bistro () (`Dry_run dry_run) (`Package_names pkg_names)
     (`Package_version version) (`Dist_tag tag) (`Keep_v keep_v) (`Token token)
     (`Include_submodules include_submodules) (`Draft draft)
-    (`Local_repo local_repo) (`Remote_repo remote_repo) (`Opam_repo opam_repo) =
+    (`Local_repo local_repo) (`Remote_repo remote_repo) (`Opam_repo opam_repo)
+    (`No_auto_open no_auto_open) =
   Cli.handle_error
-    ( Dune_release.Config.keep_v keep_v >>= fun keep_v ->
-      Dune_release.Config.token ?cli_token:token ~dry_run () >>= fun token ->
+    ( Dune_release.Config.token ~token ~dry_run () >>= fun token ->
+      let token = Dune_release.Config.Cli.make token in
       Distrib.distrib ~dry_run ~pkg_names ~version ~tag ~keep_v ~keep_dir:false
         ~skip_lint:false ~skip_build:false ~skip_tests:false ~include_submodules
         ()
@@ -25,8 +26,8 @@ let bistro () (`Dry_run dry_run) (`Package_names pkg_names)
       >! fun () ->
       Opam.get_pkgs ~dry_run ~keep_v ~tag ~pkg_names ~version () >>= fun pkgs ->
       Opam.pkg ~dry_run ~pkgs () >! fun () ->
-      Opam.submit ~token ~dry_run ~pkgs ~pkg_names ~no_auto_open:false
-        ~yes:false ~draft () ?local_repo ?remote_repo ?opam_repo )
+      Opam.submit ~token ~dry_run ~pkgs ~pkg_names ~no_auto_open ~yes:false
+        ~draft () ?local_repo ?remote_repo ?opam_repo )
 
 (* Command line interface *)
 
@@ -56,7 +57,8 @@ let cmd =
   ( Term.(
       pure bistro $ Cli.setup $ Cli.dry_run $ Cli.pkg_names $ Cli.pkg_version
       $ Cli.dist_tag $ Cli.keep_v $ Cli.token $ Cli.include_submodules
-      $ Cli.draft $ Cli.local_repo $ Cli.remote_repo $ Cli.opam_repo),
+      $ Cli.draft $ Cli.local_repo $ Cli.remote_repo $ Cli.opam_repo
+      $ Cli.no_auto_open),
     Term.info "bistro" ~doc ~sdocs ~exits ~man ~man_xrefs )
 
 (*---------------------------------------------------------------------------
diff --git a/bin/check.ml b/bin/check.ml
index a8bf6bfd..c4262d78 100644
--- a/bin/check.ml
+++ b/bin/check.ml
@@ -41,6 +41,7 @@ let check (`Package_names pkg_names) (`Package_version version) (`Dist_tag tag)
        (dir, clean_up)
    in
    dir >>= fun dir ->
+   Config.keep_v ~keep_v >>= fun keep_v ->
    let check_result =
      Check.check_project ~pkg_names ?tag ?version ~keep_v ?build_dir ~skip_lint
        ~skip_build ~skip_tests ~dir ()
diff --git a/bin/cli.ml b/bin/cli.ml
index 1b48b4ad..5abfa2a2 100644
--- a/bin/cli.ml
+++ b/bin/cli.ml
@@ -21,6 +21,12 @@ let dir_path_arg =
 
 let named f = Cmdliner.Term.(app (const f))
 
+let config term = Cmdliner.Term.(app (const Dune_release.Config.Cli.make)) term
+
+let config_opt term =
+  let open Cmdliner.Term in
+  app (const (Stdext.Option.map ~f:Dune_release.Config.Cli.make)) term
+
 let dist_tag =
   let doc =
     "The tag from which the distribution archive is or will be built."
@@ -71,13 +77,20 @@ let token =
   in
   let docv = "TOKEN" in
   let env = Arg.env_var "DUNE_RELEASE_GITHUB_TOKEN" in
-  named
-    (fun x -> `Token x)
+  let arg =
     Arg.(value & opt (some string) None & info [ "token" ] ~doc ~docv ~env)
+  in
+  named (fun x -> `Token x) (config_opt arg)
 
 let keep_v =
   let doc = "Do not drop the initial 'v' in the version string." in
-  named (fun x -> `Keep_v x) Arg.(value & flag & info [ "keep-v" ] ~doc)
+  let arg = Arg.(value & flag & info [ "keep-v" ] ~doc) in
+  named (fun x -> `Keep_v x) (config arg)
+
+let no_auto_open =
+  let doc = "Do not open a browser to view the new pull-request." in
+  let arg = Arg.(value & flag & info [ "no-auto-open" ] ~doc) in
+  named (fun x -> `No_auto_open x) (config arg)
 
 let dist_file =
   let doc =
@@ -189,22 +202,24 @@ let user =
 let local_repo =
   let doc = "Location of the local fork of opam-repository" in
   let env = Arg.env_var "DUNE_RELEASE_LOCAL_REPO" in
-  named
-    (fun x -> `Local_repo x)
+  let arg =
     Arg.(
       value
       & opt (some dir_path_arg) None
       & info ~env [ "l"; "local-repo" ] ~doc ~docv:"PATH")
+  in
+  named (fun x -> `Local_repo x) (config_opt arg)
 
 let remote_repo =
   let doc = "Location of the remote fork of opam-repository" in
   let env = Arg.env_var "DUNE_RELEASE_REMOTE_REPO" in
-  named
-    (fun x -> `Remote_repo x)
+  let arg =
     Arg.(
       value
       & opt (some string) None
       & info ~env [ "r"; "remote-repo" ] ~doc ~docv:"URI")
+  in
+  named (fun x -> `Remote_repo x) (config_opt arg)
 
 let opam_repo =
   let doc =
diff --git a/bin/cli.mli b/bin/cli.mli
index e9b253e0..c737bafc 100644
--- a/bin/cli.mli
+++ b/bin/cli.mli
@@ -19,13 +19,17 @@ val named : ('a -> 'b) -> 'a Cmdliner.Term.t -> 'b Cmdliner.Term.t
     avoid confusion when they are later passed to your main function. Example:
     [named (fun x -> `My_arg x) Arg.(value ...)] *)
 
+val no_auto_open : [ `No_auto_open of bool Dune_release.Config.Cli.t ] Term.t
+(** A [--no-auto-open] option to disable opening of the opam-repository PR in
+    the browser. *)
+
 val pkg_names : [ `Package_names of string list ] Term.t
 (** A [--pkg-names] option to specify the packages to release. *)
 
 val pkg_version : [ `Package_version of string option ] Term.t
 (** A [--pkg-version] option to specify the packages version. *)
 
-val keep_v : [ `Keep_v of bool ] Term.t
+val keep_v : [ `Keep_v of bool Dune_release.Config.Cli.t ] Term.t
 (** A [--keep-v] option to not drop the 'v' at the beginning of version strings. *)
 
 val dist_tag : [ `Dist_tag of string option ] Term.t
@@ -56,7 +60,7 @@ val build_dir : [ `Build_dir of Fpath.t option ] Term.t
 val publish_msg : [ `Publish_msg of string option ] Term.t
 (** A [--msg] option to define a publication message. *)
 
-val token : [ `Token of string option ] Term.t
+val token : [ `Token of string Dune_release.Config.Cli.t option ] Term.t
 (** A [--token] option to define the github token. *)
 
 val dry_run : [ `Dry_run of bool ] Term.t
@@ -75,11 +79,13 @@ val user : [ `User of string option ] Term.t
 (** A [--user] option to define the name of the GitHub account where to push new
     opam-repository branches. *)
 
-val local_repo : [ `Local_repo of Fpath.t option ] Term.t
+val local_repo :
+  [ `Local_repo of Fpath.t Dune_release.Config.Cli.t option ] Term.t
 (** A [--local-repo] option to define the location of the local fork of
     opam-repository. *)
 
-val remote_repo : [ `Remote_repo of string option ] Term.t
+val remote_repo :
+  [ `Remote_repo of string Dune_release.Config.Cli.t option ] Term.t
 (** A [--remote-repo] option to define the location of the remote fork of
     opam-repository. *)
 
diff --git a/bin/distrib.ml b/bin/distrib.ml
index 368e30ef..4b8bc3b5 100644
--- a/bin/distrib.ml
+++ b/bin/distrib.ml
@@ -44,7 +44,7 @@ let distrib ?build_dir ~dry_run ~pkg_names ~version ~tag ~keep_v ~keep_dir
     ~skip_lint ~skip_build ~skip_tests ~include_submodules () =
   App_log.status (fun l -> l "Building source archive");
   warn_if_vcs_dirty () >>= fun () ->
-  Config.keep_v keep_v >>= fun keep_v ->
+  Config.keep_v ~keep_v >>= fun keep_v ->
   let pkg = Pkg.v ~dry_run ?version ~keep_v ?build_dir ?tag () in
   Pkg.distrib_archive ~dry_run ~keep_dir ~include_submodules pkg >>= fun ar ->
   log_wrote_archive ar >>= fun () ->
diff --git a/bin/distrib.mli b/bin/distrib.mli
index 0077e041..7a24087d 100644
--- a/bin/distrib.mli
+++ b/bin/distrib.mli
@@ -12,7 +12,7 @@ val distrib :
   pkg_names:string list ->
   version:string option ->
   tag:string option ->
-  keep_v:bool ->
+  keep_v:bool Dune_release.Config.Cli.t ->
   keep_dir:bool ->
   skip_lint:bool ->
   skip_build:bool ->
diff --git a/bin/lint.ml b/bin/lint.ml
index 006ab706..a2e434ac 100644
--- a/bin/lint.ml
+++ b/bin/lint.ml
@@ -10,7 +10,7 @@ open Dune_release
 let lint () (`Dry_run dry_run) (`Package_names pkg_names)
     (`Package_version version) (`Dist_tag tag) (`Keep_v keep_v) (`Lints lints) =
   Cli.handle_error
-    ( Config.keep_v keep_v >>= fun keep_v ->
+    ( Config.keep_v ~keep_v >>= fun keep_v ->
       let pkg = Pkg.v ~dry_run ?version ~keep_v ?tag () in
       OS.Dir.current () >>= fun dir ->
       Lint.lint_packages ~dry_run ~dir ~todo:lints pkg pkg_names )
diff --git a/bin/opam.ml b/bin/opam.ml
index 16446c07..e091f02d 100644
--- a/bin/opam.ml
+++ b/bin/opam.ml
@@ -256,7 +256,7 @@ let field pkgs field =
 
 let get_pkgs ?build_dir ?opam ?distrib_file ?readme ?change_log ?publish_msg
     ?pkg_descr ~dry_run ~keep_v ~tag ~pkg_names ~version () =
-  Config.keep_v keep_v >>= fun keep_v ->
+  Config.keep_v ~keep_v >>= fun keep_v ->
   let distrib_file =
     let pkg =
       Pkg.v ?opam ?tag ?version ?distrib_file ~dry_run:false ~keep_v ()
@@ -293,9 +293,9 @@ let submit ?local_repo:local ?remote_repo:remote ?opam_repo ?user ?token
     match opam_repo with None -> ("ocaml", "opam-repository") | Some r -> r
   in
   report_user_option_use user;
-  Config.token ?cli_token:token ~dry_run () >>= fun token ->
+  Config.token ~token ~dry_run () >>= fun token ->
   Config.opam_repo_fork ~pkgs ~local ~remote () >>= fun { remote; local } ->
-  Config.auto_open (not no_auto_open) >>= fun auto_open ->
+  Config.auto_open ~no_auto_open >>= fun auto_open ->
   App_log.status (fun m ->
       m "Submitting %a" Fmt.(list ~sep:sp Text.Pp.name) pkg_names);
   submit ~token ~dry_run ~yes ~opam_repo local remote pkgs auto_open ~draft
@@ -345,12 +345,6 @@ let field_arg =
     (fun x -> `Field_name x)
     Arg.(value & pos 1 (some string) None & info [] ~doc ~docv:"FIELD")
 
-let no_auto_open =
-  let doc = "Do not open a browser to view the new pull-request." in
-  Cli.named
-    (fun x -> `No_auto_open x)
-    Arg.(value & flag & info [ "no-auto-open" ] ~doc)
-
 let pkg_descr =
   let doc =
     "The opam descr file to use for the opam package. If absent and the opam \
@@ -413,7 +407,7 @@ let cmd =
       $ Cli.remote_repo $ Cli.opam_repo $ Cli.user $ Cli.keep_v $ Cli.dist_opam
       $ Cli.dist_uri $ Cli.dist_file $ Cli.dist_tag $ Cli.pkg_names
       $ Cli.pkg_version $ pkg_descr $ Cli.readme $ Cli.change_log
-      $ Cli.publish_msg $ action $ field_arg $ no_auto_open $ Cli.yes
+      $ Cli.publish_msg $ action $ field_arg $ Cli.no_auto_open $ Cli.yes
       $ Cli.token $ Cli.draft)
   in
   (t, info)
diff --git a/bin/opam.mli b/bin/opam.mli
index c71f037a..15c98f3d 100644
--- a/bin/opam.mli
+++ b/bin/opam.mli
@@ -15,7 +15,7 @@ val get_pkgs :
   ?publish_msg:string ->
   ?pkg_descr:Fpath.t ->
   dry_run:bool ->
-  keep_v:bool ->
+  keep_v:bool Dune_release.Config.Cli.t ->
   tag:string option ->
   pkg_names:string list ->
   version:string option ->
@@ -41,15 +41,15 @@ val pkg :
     for success, 1 for failure) or error messages. *)
 
 val submit :
-  ?local_repo:Fpath.t ->
-  ?remote_repo:string ->
+  ?local_repo:Fpath.t Dune_release.Config.Cli.t ->
+  ?remote_repo:string Dune_release.Config.Cli.t ->
   ?opam_repo:string * string ->
   ?user:string ->
-  ?token:string ->
+  ?token:string Dune_release.Config.Cli.t ->
   dry_run:bool ->
   pkgs:Dune_release.Pkg.t list ->
   pkg_names:string list ->
-  no_auto_open:bool ->
+  no_auto_open:bool Dune_release.Config.Cli.t ->
   yes:bool ->
   draft:bool ->
   unit ->
diff --git a/bin/publish.ml b/bin/publish.ml
index 6c0def24..577e901e 100644
--- a/bin/publish.ml
+++ b/bin/publish.ml
@@ -80,7 +80,7 @@ let publish ?build_dir ?opam ?delegate ?change_log ?distrib_uri ?distrib_file
   let publish_artefacts =
     match publish_artefacts with [] -> [ `Doc; `Distrib ] | v -> v
   in
-  Config.keep_v keep_v >>= fun keep_v ->
+  Config.keep_v ~keep_v >>= fun keep_v ->
   let pkg =
     Pkg.v ~dry_run ?version ?tag ~keep_v ?build_dir ?opam ?change_log
       ?distrib_file ?publish_msg ?delegate ()
diff --git a/bin/publish.mli b/bin/publish.mli
index e3402252..4bb79efc 100644
--- a/bin/publish.mli
+++ b/bin/publish.mli
@@ -14,11 +14,11 @@ val publish :
   ?distrib_uri:string ->
   ?distrib_file:Fpath.t ->
   ?publish_msg:string ->
-  ?token:string ->
+  ?token:string Dune_release.Config.Cli.t ->
   pkg_names:string list ->
   version:string option ->
   tag:string option ->
-  keep_v:bool ->
+  keep_v:bool Dune_release.Config.Cli.t ->
   dry_run:bool ->
   publish_artefacts:[ `Alt of string | `Distrib | `Doc ] list ->
   yes:bool ->
diff --git a/bin/undraft.ml b/bin/undraft.ml
index f24cff47..49adce7a 100644
--- a/bin/undraft.ml
+++ b/bin/undraft.ml
@@ -41,7 +41,7 @@ let update_opam_file ~dry_run ~url pkg =
 
 let undraft ?opam ?distrib_file ?opam_repo ?token ?local_repo:local
     ?remote_repo:remote ?build_dir ?pkg_names ~dry_run ~yes:_ () =
-  Config.token ?cli_token:token ~dry_run () >>= fun token ->
+  Config.token ~token ~dry_run () >>= fun token ->
   let pkg = Pkg.v ?opam ?distrib_file ?build_dir ~dry_run:false () in
   Config.opam_repo_fork ~pkgs:[ pkg ] ~local ~remote ()
   >>= fun opam_repo_fork ->
diff --git a/lib/config.ml b/lib/config.ml
index a7441cfe..c2def753 100644
--- a/lib/config.ml
+++ b/lib/config.ml
@@ -230,22 +230,30 @@ let config_token ~dry_run () =
       OS.Dir.create Fpath.(parent file) >>= fun _ ->
       OS.File.write ~mode:0o600 file token >>= fun () -> Ok token
 
-let token ?cli_token ~dry_run () =
-  match cli_token with
+module Cli = struct
+  type 'a t = 'a
+
+  let make x = x
+end
+
+let token ~token ~dry_run () =
+  match token with
   | Some _ when dry_run -> Ok Dry_run.token
   | Some token -> Ok token
   | None -> config_token ~dry_run ()
 
 let file = lazy (find ())
 
-let read f default =
+let read f ~default =
   Lazy.force file >>| function
   | None -> default
   | Some t -> ( match f t with None -> default | Some b -> b)
 
-let keep_v v = if v then Ok true else read (fun t -> t.keep_v) false
+let keep_v ~keep_v =
+  if keep_v then Ok true else read (fun t -> t.keep_v) ~default:false
 
-let auto_open v = if not v then Ok false else read (fun t -> t.auto_open) true
+let auto_open ~no_auto_open =
+  if no_auto_open then Ok false else read (fun t -> t.auto_open) ~default:true
 
 let opam_repo_fork ?pkgs ~remote ~local () =
   match (remote, local) with
diff --git a/lib/config.mli b/lib/config.mli
index 4f0c421a..d8888202 100644
--- a/lib/config.mli
+++ b/lib/config.mli
@@ -28,23 +28,33 @@ end
 
 val create : ?pkgs:Pkg.t list -> unit -> (unit, Bos_setup.R.msg) result
 
+module Cli : sig
+  type 'a t
+  (** Type for configuration values passed through the CLI. *)
+
+  val make : 'a -> 'a t
+end
+
 val token :
-  ?cli_token:string -> dry_run:bool -> unit -> (string, Bos_setup.R.msg) result
+  token:string Cli.t option ->
+  dry_run:bool ->
+  unit ->
+  (string, Bos_setup.R.msg) result
 (** Returns the token value that should be used for github API requests. If a
-    [cli_token] was provided, it is returned. Otherwise the token file in the
-    config dir is looked up. If it exists, its content is returned, if it does
-    not, the user is prompted for a token which will be then saved to that file.
-    When [dry_run] is [true] it always returns [Ok "${token}"] but still looks
-    up the relevant config file as it would normally have. *)
+    [token] was provided via the CLI, it is returned. Otherwise the token file
+    in the config dir is looked up. If it exists, its content is returned, if it
+    does not, the user is prompted for a token which will be then saved to that
+    file. When [dry_run] is [true] it always returns [Ok "${token}"] but still
+    looks up the relevant config file as it would normally have. *)
 
-val keep_v : bool -> (bool, Bos_setup.R.msg) result
+val keep_v : keep_v:bool Cli.t -> (bool, Bos_setup.R.msg) result
 
-val auto_open : bool -> (bool, Bos_setup.R.msg) result
+val auto_open : no_auto_open:bool Cli.t -> (bool, Bos_setup.R.msg) result
 
 val opam_repo_fork :
   ?pkgs:Pkg.t list ->
-  remote:string option ->
-  local:Fpath.t option ->
+  remote:string Cli.t option ->
+  local:Fpath.t Cli.t option ->
   unit ->
   (Opam_repo_fork.t, Bos_setup.R.msg) result
 (** Returns the opam-repository fork to use, based on the CLI provided values
diff --git a/lib/delegate.ml b/lib/delegate.ml
index 85a42124..ac6ff16e 100644
--- a/lib/delegate.ml
+++ b/lib/delegate.ml
@@ -22,7 +22,7 @@ let publish_distrib ?token ?distrib_uri ~dry_run ~msg ~archive ~yes ~draft pkg =
   Pkg.delegate pkg >>= function
   | None ->
       App_log.status (fun l -> l "Publishing to github");
-      Config.token ?cli_token:token ~dry_run () >>= fun token ->
+      Config.token ~token ~dry_run () >>= fun token ->
       Github.publish_distrib ~token ~dry_run ~yes ~msg ~archive ~draft pkg
       >>= fun url ->
       Pkg.archive_url_path pkg >>= fun url_file ->
diff --git a/lib/delegate.mli b/lib/delegate.mli
index eed0dcfb..46283ef2 100644
--- a/lib/delegate.mli
+++ b/lib/delegate.mli
@@ -13,7 +13,7 @@ open Bos_setup
 (** {1 Publish} *)
 
 val publish_distrib :
-  ?token:string ->
+  ?token:string Config.Cli.t ->
   ?distrib_uri:string ->
   dry_run:bool ->
   msg:string ->