-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
cli: do not double quote chromeFlags #3775
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.
:)
So basically it's taken care of since spawn
passes in arguments as an array, so they don't need their values quoted? Or something else? This is confusing because the fix in #2754 needed quotes, but maybe the spawn method or our handling of chromeFlags
has changed since then.
I tried out the problem case from that PR (--chrome-flags="--user-agent=\"iPhone Test\""
) and it works just fine with this PR.
👍 for the explanatory comment, but maybe the tests can do with some cleaning up? Some repetition now and not all the descriptions match up to their test (e.g. quotes flag values with spaces in them
)
Link just takes me to the commit so not entirely sure if you're talking about #3408 (comment), but the short story here is that quoting any value is indeed bad, not that quoting single words is bad but spaced values is fine.
Correct
Nothing has changed, but it is confusing. #2754 was about us needing to respect quoted values when parsing because we just did a basic All of this would go away if we weren't using spawn and just doing a shell
ya done |
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.
LGTM
appveyor is being dumb missing that offscreen svg again. Ignoring for now |
When could this merge be published? I've encountered the |
closes #3744