-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: fix positional args in task runner #52810
Conversation
cf0ccb9
to
cf8fef9
Compare
Windows CI is failing:
|
@nodejs/platform-windows @lemire I couldn't find the reason for the failing test case. It seems the first argument receives an unnecessary quote before after it which causes the issue fail only on Windows. Any idea why it's happening and how to avoid it? |
On macOS, running the following command produces the same exact thing:
But on Windows, the |
@anonrig Windows programming is a very ancient art passed on from generation to generation. Looking... |
It produces something entirely different on Windows... I don't understand.
|
On Windows this branch produces the following output:
|
Probably related to #52682 (comment) / npm/cli#7375. |
@richardlau It seems ditching |
@anonrig I am trying to understand your test in the first place. Normally, If you are considering them as just one, then it is an argument with a space. So the first argument contains a space, correct? So maybe it is "C:\my document\file.txt". Right? It is the same idea. That is, a file with a space. So you want to call the script with the first argument being "C:\my document\file.txt". Correct? Now if you end up with
Then that's clearly wrong. You need quotes or something else around it. Are we agreed? |
@lemire EscapeShell function adds quotes around any argument that has space. But somehow that quotation marks are removed on Windows. |
Under Windows, are single quotes the correct approach? Do you have a reference? |
So... if you do
The there are two parameters...
and
Are we agreed? |
You get back...
which is...
|
@lemire I don't have any. Maybe it is wrong. I was following the original npm implementation, which is also flawed I guess. |
@anonrig I'll issue a PR against your PR. |
The reference is here: https://daviddeley.com/autohotkey/parameters/parameters.htm |
e4e673f
to
4e18873
Compare
@lemire I fixed 2 issues, but there is still an unnecessary
|
@anonrig It says that there are six arguments and prints |
|
2f2f16f
to
f388eec
Compare
Landed in fe4e569 |
PR-URL: nodejs#52810 Fixes: nodejs#52740 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#52810 Fixes: nodejs#52740 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The proposed change correctly splits the positional arguments.
Fixes: #52740