-
Notifications
You must be signed in to change notification settings - Fork 450
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
Prioritizing --help
arg handling
#6667
Conversation
It should, but didn't maybe we just need to avoid to use spawnSync in tests |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
6d3f8f9
to
709d202
Compare
709d202
to
6098902
Compare
acd2ca7
to
321013c
Compare
7abc713
to
09f6741
Compare
4d37a0f
to
e4134be
Compare
e4134be
to
3d47b87
Compare
This reverts commit e4069e1.
accc231
to
f3bbcb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work fixing the build getting stuck! 👍
Some remarks:
SIGTERM: 15, | ||
}; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could add a comment here explaining why we need this function instead of spawnSync
/ what issue it fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an asynchronous version of spawnSync, with no special behavior. But preferred than spawnSync
because spawnSync
is unstable for unknown reason.
I think it would be better to add some explanation onto execReScript
util you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do it there, too.
async function test() { | ||
{ | ||
// Shows build help with --help arg | ||
const out = await exec(`../../../rescript`, ["build", "--help"], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are already refactoring this, we could add a helper function in this file so that we can just write
const out = await execReScript(["build", "--help"]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I separate it into a separate pure refactor PR?
Aside from the cli_help test, there are many other places to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course!
I'll take a look during the day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with FS api, but logic wise the code looks good 👍
cc @DZakh
This also indirectly fixes the indeterministic build timeout on the master branch