search

ocaml-ppx / ocaml-migrate-parsetree

DEPRECATED. See https://ocaml.org/changelog/2023-10-23-omp-deprecation. Convert OCaml parsetrees between different major versions
Other
87 stars 43 forks source link

Fix parse_argv usage #83

Closed aantron closed 4 years ago

aantron commented 5 years ago

This was just merged in #82. However, the new code is not quite correct:

  1. parse_argv raises exceptions instead of internally executing exit.
  2. parse_argv should be called with ~current:(ref 0).

Perhaps the right way to do this is to restore the former signature of run_main, and leave it as the function that calls exit, and provide a new function run_main_with_argv that will take an argv argument and raise exceptions. I actually would prefer run_main_with_argv not call exit, because that allows linking a PPX directly into a test suite, which improves testing performance.

Another option is to leave run_main with ?argv, but it will be have differently, depending on whether ~argv is supplied: if not supplied, run_main will call exit, and if supplied, it will raise exceptions. This could be surprising to users of the API, however.

@diml, what do you think about the last two paragraphs?

I'll be happy to submit a PR, considering your preference.

aantron commented 5 years ago

I went with the first option in #84, because three functions need the exit behavior change, and it is easier to have three optional arguments, than duplicate all three functions and have six entry points.