-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add a way to control which scripts get args and where #12086
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
Conversation
I think this would solve the issue nicely. I would expect the additional args to only be forwarded where I denoted it. In a future version with breaking changes it may be worth revisiting and never forwarding additional args unless explicitly including |
@naderman any opinion on this? I'd tend towards removing this automatic behavior and making it explicit only. Also what about naming, any feedback on the tag names? |
I think the magical behavior of not sending args if at least one command in the script requires args explicitly is going to be too confusing. It'll be odd that you remove that one command and suddenly all commands receive args. It's also inconsistent then with how this would work for single command scripts, and I'd rather keep BC here. So I think it's fine to require explicit @no_additional_args rather than magically removing them if something else explicitly asks for them. |
9aa57b9
to
1c6b181
Compare
1c6b181
to
c0e4fb9
Compare
amazing, thanks @Seldaek just my two cents - this can be expanded with shell-like syntax for arguments (I find {
"scripts": {
"command-name": [
"@composer install --no-dev @no_args",
"@php runner.php command-name @args[1]",
"@php runner.php @args[1] @args[2] command-name #first two args",
"@php runner.php @args[*] command-name #all args",
"@php runner.php @args[0] #the name of the command"
]
}
} |
Yeah we could, but seems a bit overkill tbh.. The main thing I wanted to do was support And yes the name is verbose and long to ensure there is no clash with anything. |
fixes #10734
This is so far fairly experimental/MVP.
Using the example from #10734 (comment) as a base, I got this test case:
Then running that script outputs: