Fix compile status with help command #6439

DZakh · 2023-10-13T11:27:13Z

Fixes: #6435 (comment) (only the comment)

jscomp/build_tests/cli_help/input.js

DZakh · 2023-10-13T11:30:57Z

rescript

+  const isDefinitelyBuild =
+    maybeSubcommand === undefined ||
+    (maybeSubcommand === "build" && process_argv.length === 3);
+
+  if (isDefinitelyBuild) {
+    logStartCompiling();
+    delegateCommand(process_argv.slice(2), logFinishCompiling);
+  } else {


A little bit hacky, but I think better, than nothing. Ideally, to move all the args parsing logic on the js side in one place.

https://nodejs.org/api/util.html#utilparseargsconfig will be useful if we accept #6429

I was actually thinking of using minimist, but https://github.com/pkgjs/parseargs also looks good.

If we are more tolerant of external dependencies we can even import more, like clipanion 😂

zth · 2023-10-21T11:05:43Z

@cknitt you've been reviewing changes here before, up for reviewing this too?

cknitt

Thanks for adding those tests, this is great! 👍

jscomp/build_tests/cli_help/input.js

rescript

DZakh · 2023-10-23T08:06:46Z

Thanks for feedback, I'll return back with the improvements

DZakh · 2023-10-26T14:12:36Z

Should we use status 1 or 2 for the unknown arg error?

DZakh · 2023-10-26T14:51:50Z

@cknitt It's ready for re review

DZakh · 2023-10-26T14:52:28Z

jscomp/ext/bsc_args.ml

-      b +> "unknown option: '";
+      b +> "Unknown option \"";
      b +> s;
-      b +> "'.\n"
+      b +> "\".\n"
  | Missing s ->
-      b +> "option '";
+      b +> "Option \"";
      b +> s;
-      b +> "' needs an argument.\n");


Actually, the code is not used in my case. I've just decided to make it consistent with bsb_arg.ml.

DZakh · 2023-10-26T14:52:42Z

rescript

+        break;
+      case "-h":
+      case "-help":
+      case "--help":


Added --help here

DZakh · 2023-10-26T14:52:56Z

rescript

+      case "--version":
+      case "version":


Added --version and version here

DZakh · 2023-10-26T14:54:02Z

rescript

+        console.error(
+          `Error: Unknown command or flag "${maybeSubcommand}".\n${helpMessage}`
+        );


Now everything will come to stderr. Before helpMessage was shown in stdout while Unknown command or flag in stdout. That's also the reason why I have both stdout and stderr check in the tests.

DZakh · 2023-10-26T14:55:24Z

jscomp/build_tests/cli_help/input.js

+// FIXME: Help works incorrectly in watch mode
+out = child_process.spawnSync(`../../../rescript`, ["build", "-w", "--help"], {
+  encoding: "utf8",
+  cwd: __dirname,
+});
+// FIXME: Shouldn't have "Start compiling" for help
+assert.equal(out.stdout, ">>>> Start compiling\n" + buildHelp);
+// FIXME: Don't run the watcher when showing help
+assert.match(
+  out.stderr,
+  new RegExp(
+    "Uncaught Exception Error: ENOENT: no such file or directory, watch 'bsconfig.json'\n"
+  )
+);
+// FIXME: Should be 0
+assert.equal(out.status, 1);


It'll be only possible to fix by moving the args parsing logic in one place. I want to do it when I have more free time.

What about the other three FIXMEs above? Also something for later?

Yeah, they are all caused by one problem

cknitt · 2023-10-27T11:33:39Z

jscomp/build_tests/cli_help/input.js

+  "\n" +
+  "`rescript build` builds the project with dependencies\n" +
+  "\n" +
+  "`rescript -- -h` for Ninja options (internal usage only; unstable)\n" +


Should be rescript build -- -h.

Good to have tests 😁 Fixed 👌

cknitt · 2023-10-27T11:36:14Z

jscomp/build_tests/cli_help/input.js

+// FIXME: Help works incorrectly in watch mode
+out = child_process.spawnSync(`../../../rescript`, ["build", "-w", "--help"], {
+  encoding: "utf8",
+  cwd: __dirname,
+});
+// FIXME: Shouldn't have "Start compiling" for help
+assert.equal(out.stdout, ">>>> Start compiling\n" + buildHelp);
+// FIXME: Don't run the watcher when showing help
+assert.match(
+  out.stderr,
+  new RegExp(
+    "Uncaught Exception Error: ENOENT: no such file or directory, watch 'bsconfig.json'\n"
+  )
+);
+// FIXME: Should be 0
+assert.equal(out.status, 1);


What about the other three FIXMEs above? Also something for later?

cknitt · 2023-10-28T14:59:27Z

@DZakh Just wanted to merge this for rc.5, but there are conflicts, could you resolve them?

DZakh · 2023-10-28T19:48:21Z

I'll fix on Monday

DZakh commented Oct 13, 2023

View reviewed changes

DZakh changed the title ~~Fx compile status with help command~~ Fix compile status with help command Oct 17, 2023

cknitt reviewed Oct 22, 2023

View reviewed changes

jscomp/build_tests/cli_help/input.js Outdated Show resolved Hide resolved

jscomp/build_tests/cli_help/input.js Outdated Show resolved Hide resolved

jscomp/build_tests/cli_help/input.js Outdated Show resolved Hide resolved

rescript Outdated Show resolved Hide resolved

DZakh force-pushed the fix-compile-status-in-help branch from b8b680c to 514b317 Compare October 26, 2023 14:46

DZakh commented Oct 26, 2023

View reviewed changes

rescript

break;

case "-h":

case "-help":

case "--help":

Copy link

Contributor Author

DZakh Oct 26, 2023

Choose a reason for hiding this comment

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

Added --help here

DZakh commented Oct 26, 2023

View reviewed changes

DZakh requested a review from cknitt October 26, 2023 14:55

cknitt reviewed Oct 27, 2023

View reviewed changes

cknitt approved these changes Oct 28, 2023

View reviewed changes

DZakh added 7 commits October 30, 2023 17:08

Fx compile status with help command

c6de8a7

Update changelog

8396247

Update tests

bbaf8ca

Improve cli consistency

71ec60a

Finish cli help consistency improvements

df001a7

Update changelog

b04113b

Fix build help

f1d4059

DZakh force-pushed the fix-compile-status-in-help branch from ab20cdb to f1d4059 Compare October 30, 2023 13:10

cknitt approved these changes Oct 30, 2023

View reviewed changes

cknitt merged commit 5b2c5bd into rescript-lang:master Oct 30, 2023
14 checks passed

fhammerschmidt mentioned this pull request Nov 8, 2023

Document v11 features. rescript-lang/rescript-lang.org#697

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compile status with help command #6439

Fix compile status with help command #6439

DZakh commented Oct 13, 2023 •

edited

Loading

DZakh Oct 13, 2023

cometkim Oct 18, 2023

DZakh Oct 18, 2023

cometkim Oct 18, 2023

zth commented Oct 21, 2023

cknitt left a comment

DZakh commented Oct 23, 2023

DZakh commented Oct 26, 2023

DZakh commented Oct 26, 2023

DZakh Oct 26, 2023

DZakh Oct 26, 2023

DZakh Oct 26, 2023 •

edited

Loading

DZakh Oct 26, 2023

DZakh Oct 26, 2023

cknitt Oct 27, 2023

DZakh Oct 27, 2023 •

edited

Loading

cknitt Oct 27, 2023

DZakh Oct 27, 2023

cknitt Oct 27, 2023

cknitt commented Oct 28, 2023

DZakh commented Oct 28, 2023

Fix compile status with help command #6439

Fix compile status with help command #6439

Conversation

DZakh commented Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented Oct 21, 2023

cknitt left a comment

Choose a reason for hiding this comment

DZakh commented Oct 23, 2023

DZakh commented Oct 26, 2023

DZakh commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DZakh Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DZakh Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cknitt commented Oct 28, 2023

DZakh commented Oct 28, 2023

DZakh commented Oct 13, 2023 •

edited

Loading

DZakh Oct 26, 2023 •

edited

Loading

DZakh Oct 27, 2023 •

edited

Loading