-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement specification of subcommand #854
Conversation
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.
Long comment. 🙂
I have not done run-time testing yet, this is some initial comments.
I would like to eventually add a routine to match .action
where this configuration could go, but that is a bigger job, and I assume you don't want to wait for that, which is ok!
I like that you added the support in the existing .command
options so does not affecting existing code.
Now for the requested change suggestion: I do not like subcommand
as the option name.
We already have a lot of confusion over the two modes so we need a name to make it clearer that this is not relevant to .action
based commands. I have been thinking about naming and considering words like "spawn" and "exec" and "external". I checked how people describe git subcommands and found "Git subcommands are standalone executables" which I thought was fairly clear.
Perhaps: "executableFile" instead of "subcommand"?
(I am not sure about "File" here, but came to that partly because node child_process has execFile. I will ask for comments on naming in the linked issue too.)
To have behaviour consistent, I think we should calculate The behaviour I see is:
|
I don't think we need to wait any longer for feedback on |
Would you like example code for the |
This is being considered for v3, but some changes suggested. I am willing to help with code if wanted (in which case I'll get it reviewed before merging). |
Started work on implementing my suggestions |
Closing in favour of #999 which builds on this pull request. |
Available now as a prerelease. See #1001 |
#532