refactor Cmdliner usage from $ to let+
#503
Conversation
| $ Cli.dev_repo) | ||
| let open Syntax in | ||
| let+ () = Cli.setup | ||
| and+ dry_run = Cli.dry_run |
There was a problem hiding this comment.
You can also use let open Cli and then and+ dry_run and+ pkg_names .... That's available starting from OCaml 4.13 and ocamlformat will auto-format it that way if you let it :-)
I'm normally not a fan of random opens but in this case might be ok (or move all the things that are meant to be opened into a submodule O and let open Cli.O in to avoid pulling in unexpected names.
There was a problem hiding this comment.
If you want the change, I suppose I can do that while I'm at it.
But if it's merely a thought you had, I wouldn't add this kind of open in my own code (in fact I don't even want to open Cmdliner.Term in my code).
| (** A [--no-auto-open] option to disable opening of the opam-repository PR in | ||
| the browser. *) | ||
|
|
||
| val version : Dune_release.Version.t Arg.conv | ||
| (** [version] is a converter for parsing version specifiers *) | ||
|
|
||
| val pkg_names : [ `Package_names of string list ] Term.t | ||
| val pkg_names : string list Term.t |
There was a problem hiding this comment.
I will say that this is quite a bit nicer, I like it.
Leonidas-from-XIV
added
the
no changelog
v-gb
force-pushed
the
refactor-cmdliner
branch
from
7ee17d5 to
5f2f47e
Compare
v-gb
force-pushed
the
refactor-cmdliner
branch
from
5f2f47e to
ca2ce09
Compare
|
I pushed a new version of the commits. [1] concretely, since there was no opportunity for a bug in the code, I added one in 96663bf, and one can see what happens to |
This change was made purely mechanically, with:
ocamlmig replace \
-e 'const [%move_def __f] /// const __f' \
-e 'const (fun __p1 __p2 __p3 -> __body) $ __e1 $ __e2 $ __e3
/// let open Syntax in let+ __p1 = __e1 and+ __p2 = __e2 and+ __p3 = __e3 in __body' \
-w
These are only necessary because the $ interface is so error prone otherwise. Since we don't use the $ interface anymore, these tags are unnecessary. This change is purely mechanical, and made with: ocamlmig replace \ bin/*.ml bin/cli.mli \ -e '(Cli.named or named) __ __e /// __e' \ -e 'type: [ `X__ of __t ] /// __t' \ -e 'type: [> `X__ of __t ] /// __t' \ -e 'binding: let+ `X__ __pat = __e in __ /// let+ __pat = __e in __' \ -w
I think this style is generally preferred. This change was made purely mechanically with: ocamlmig replace -e 'binding: let+ __pat = [%move_def __f] in __ /// let+ __pat = __f in __' -w
Hi,
After the recent changes where I read/touched some command line definitions of the form:
I propose to switch them all to:
which is much simpler to modify/write. This is how everything is written in dune, AFAICT.
This is probably best read commit by commit. The last one may be more contentious. This is a fair amount of churn because of code moves, however the changes were made automatically (other than a couple of lines of diff in separate commits), so there's no risk of having swapped two arguments of type string. The code moves could potentially cause identifier captures, even though the code types and the tests pass, but the tool would warn about captures (for value identifiers. For module/types/constructors/etc, the result wouldd be virtually guaranteed to be a type error).
This is a change out of the blue, but I wanted to try out automating this change regardless of whether you want to take it, so if you don't want this, I'm fine with closing.