Skip to content

Refactor CLI handling#1127

Merged
Julow merged 12 commits intoocaml-ppx:masterfrom
Julow:refactor_cli
Nov 12, 2019
Merged

Refactor CLI handling#1127
Julow merged 12 commits intoocaml-ppx:masterfrom
Julow:refactor_cli

Conversation

@Julow
Copy link
Copy Markdown
Collaborator

@Julow Julow commented Nov 5, 2019 •
edited
Loading

This PR refactors the code in Conf.ml that interprets CLI options telling OCamlformat what to do.

This code was very hard to maintain and could contain bugs. Now the code is structured in a way that "impossible" cases are not needed and subtle side effects are removed.

The diffs on the CLI tests are:

diff --git a/test/cli/test_cli.expected b/test/cli/test_cli.expected
index 06e5a24..62d5afc 100644
--- a/test/cli/test_cli.expected
+++ b/test/cli/test_cli.expected
@@ -82,39 +82,31 @@ let () = print_endline A.x
 err_stdin_name_unknown_ext
 ==========================
 args: --name b.cpp -
-ocamlformat: Must specify at least one of --name, --impl or --intf when reading from stdin
+ocamlformat: Cannot deduce file kind from passed --name. Please specify --impl or --intf
 [1]
 
 err_several_files_and_kind
 ==========================
 args: --impl --check sample/a.mli sample/b.ml
-ocamlformat: ignoring "sample/a.mli" (syntax error)
-File "sample/a.mli", line 5, characters 0-0:
-Error: Syntax error
+ocamlformat: Cannot specify --impl or --intf with multiple inputs
 [1]
 
 err_several_files_and_name
 ==========================
 args: --name foo.ml --check sample/a.mli sample/b.ml
-ocamlformat: ignoring "foo.ml" (syntax error)
-File "foo.ml", line 5, characters 0-0:
-Error: Syntax error
+ocamlformat: Cannot specify --name with multiple inputs
 [1]
 
 err_several_files_and_kind_inplace
 ==================================
 args: --impl --check sample/a.mli sample/b.ml
-ocamlformat: ignoring "sample/a.mli" (syntax error)
-File "sample/a.mli", line 5, characters 0-0:
-Error: Syntax error
+ocamlformat: Cannot specify --impl or --intf with multiple inputs
 [1]
 
 err_several_files_and_name_inplace
 ==================================
 args: --name foo.ml --check sample/a.mli sample/b.ml
-ocamlformat: ignoring "foo.ml" (syntax error)
-File "foo.ml", line 5, characters 0-0:
-Error: Syntax error
+ocamlformat: Cannot specify --name with multiple inputs
 [1]
 
 fmterr_file_and_name

It's now an error to pass --name, --impl or --intf with more than 1 input file.

It's also an error to pass --print-config with an other "action" (eg. --output, --check), is it a good idea ?

To go further, this code should be moved to an other module and the action type could be changed a little to remove an other "impossible" case in ocamlformat.ml.

@Julow Julow requested review from emillon and gpetiot November 5, 2019 16:42
@facebook-github-bot
Copy link
Copy Markdown

facebook-github-bot commented Nov 5, 2019

Hi Julow! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Copy Markdown

facebook-github-bot commented Nov 5, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jberdine
Copy link
Copy Markdown
Collaborator

jberdine commented Nov 5, 2019

Re --name with multiple inputs, see #740 (comment) for a use case.

@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented Nov 6, 2019

In case of an error, you could not see which file caused it.
Shouldn't merge-fmt call ocamlformat once for each file, so it knows which call failed and can pass different names (eg. with a prefix) ?

gpetiot
gpetiot approved these changes Nov 6, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this is a nice improvement for the validate function to distinguish the input and the action (a bit verbose but we shouldn't forget any case).
Maybe it's a good step towards splitting this file into Conf/Cli in case we want to have a pure library without the cli in the future.

@Julow Julow merged commit 45f324c into ocaml-ppx:master Nov 12, 2019
@jberdine jberdine mentioned this pull request Nov 12, 2019
Closed
jberdine added a commit that referenced this pull request Nov 13, 2019
@jberdine
Revert "Refactor CLI handling (#1127)"
2fe4b3d 
This reverts commit 45f324c.
@Julow Julow mentioned this pull request Nov 18, 2019
Merged
@Julow Julow mentioned this pull request Dec 9, 2019
Merged
bogdan2412 pushed a commit to bogdan2412/ocamlformat that referenced this pull request Mar 28, 2020
@Julow @bogdan2412
Refactor CLI handling (ocaml-ppx#1127)
884dae9 
Structure the code in a way that "impossible" cases are not needed and subtle side effects are removed.

It's now an error to pass --name, --impl or --intf with more than 1 input file.
It's also an error to pass --print-config with an other "action" (eg. --output, --check).
@jberdine jberdine mentioned this pull request Apr 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpetiot gpetiot gpetiot approved these changes

@emillon emillon Awaiting requested review from emillon

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Julow @facebook-github-bot @jberdine @gpetiot