Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Term.exit and exit_status_of_result require a unit result #124

Closed
wants to merge 1 commit into from

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Dec 22, 2020

Previously, exit allowed passing any value to it and just ignored it. This leads to various errors going undetected. e.g. this program returns a successful exit status and displays no output, which is surprising:

let revolt () = Error (`Msg "Revolt!")
open Cmdliner
let revolt_t = Term.(const revolt $ const ())
let () = Term.exit @@ Term.eval (revolt_t, Term.info "revolt")

It also causes confusion when an argument is missing, e.g.

let chorus count msg =
  for i = 1 to count do print_endline msg done

open Cmdliner

let count =
  let doc = "Repeat the message $(docv) times." in
  Arg.(value & opt int 10 & info ["c"; "count"] ~docv:"COUNT" ~doc)

let chorus_t = Term.(const chorus $ count)

let () = Term.exit @@ Term.eval (chorus_t, Term.info "count")

This program compiled without error, but produces no output.

With this change, both programs report errors at compile-time.

Previously, `exit` allowed passing any value to it and just ignored it.
This leads to various errors going undetected. e.g. this program returns
a successful exit status and displays no output, which is surprising:

    let revolt () = Error (`Msg "Revolt!")
    open Cmdliner
    let revolt_t = Term.(const revolt $ const ())
    let () = Term.exit @@ Term.eval (revolt_t, Term.info "revolt")

It also causes confusion when an argument is missing, e.g.

    let chorus count msg =
      for i = 1 to count do print_endline msg done

    open Cmdliner

    let count =
      let doc = "Repeat the message $(docv) times." in
      Arg.(value & opt int 10 & info ["c"; "count"] ~docv:"COUNT" ~doc)

    let chorus_t = Term.(const chorus $ count)

    let () = Term.exit @@ Term.eval (chorus_t, Term.info "count")

This program compiled without error, but produces no output.

With this change, both programs report errors at compile-time.
@talex5
Copy link
Contributor Author

talex5 commented Dec 22, 2020

Testing this with the packages I have installed in my switch at the moment:

  • alcotest, crowbar and opam-0install were previously correct but needed minor updates for this change.
  • hxd and duff.0.2 passed errors to cmdliner expecting them to be reported. This PR would have prevented these bugs (duff removed the executables in 0.3).
  • ocurrent and ocaml-ci were incorrectly passing errors to cmdliner, but also logged them (presumably because I'd been confused by this behaviour in the past).

talex5 added a commit to talex5/ocaml-ci that referenced this pull request Dec 22, 2020
We previously passed the error to cmdliner, but that just ignores it and
exits with success.

See dbuenzli/cmdliner#124.
talex5 added a commit to talex5/ocaml-ci that referenced this pull request Dec 22, 2020
We previously passed the error to cmdliner, but that just ignores it and
exits with success.

See dbuenzli/cmdliner#124.
talex5 added a commit to talex5/ocaml-ci that referenced this pull request Dec 22, 2020
We previously passed the error to cmdliner, but that just ignores it and
exits with success.

See dbuenzli/cmdliner#124.
talex5 added a commit to talex5/ocluster that referenced this pull request Dec 22, 2020
talex5 added a commit to talex5/ocurrent that referenced this pull request Dec 23, 2020
Also, use `Cmdliner.Term.term_result` for correct error handling
(see dbuenzli/cmdliner#124).
talex5 added a commit to talex5/ocurrent-skeleton that referenced this pull request Dec 23, 2020
Also, use `Cmdliner.Term.term_result` for correct error handling
(see dbuenzli/cmdliner#124).
talex5 added a commit to talex5/ocurrent-skeleton that referenced this pull request Dec 23, 2020
Also, use `Cmdliner.Term.term_result` for correct error handling
(see dbuenzli/cmdliner#124).
talex5 added a commit to talex5/ocurrent-skeleton that referenced this pull request Dec 23, 2020
Also, use `Cmdliner.Term.term_result` for correct error handling
(see dbuenzli/cmdliner#124).
talex5 added a commit to talex5/ocurrent-skeleton that referenced this pull request Dec 23, 2020
Also, use `Cmdliner.Term.term_result` for correct error handling
(see dbuenzli/cmdliner#124).
talex5 added a commit to talex5/opam-repo-ci that referenced this pull request Dec 23, 2020
talex5 added a commit to talex5/ocaml-base-images that referenced this pull request Dec 23, 2020
talex5 added a commit to talex5/ocaml-base-images that referenced this pull request Dec 23, 2020
@dbuenzli
Copy link
Owner

I wish I had done these right from the onset :-( I'm a bit nervous changing these things. But your bad examples are compelling enough. Your patch is in as 2f02ae4. Thanks !

@dbuenzli dbuenzli closed this Oct 18, 2021
@dbuenzli
Copy link
Owner

dbuenzli commented Nov 9, 2021

@talex5 you also found a bug in one of my programs which was using Term.exit instead of Term.exit_status :-) Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants