-
Notifications
You must be signed in to change notification settings - Fork 228
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
Please use double quotes in the generated CURL command for windows support #115
Comments
@ElleshaHackett seems like a good idea. @ahmadnassri do you have any history/justification on the single quotes? |
I plan to work on this issue, but I've found something that borrows me: https://github.com/Kong/httpsnippet/blob/master/src/helpers/shell.js#L6-L9
This utility function is used for all shell targets. I'm wondering if
Any thoughts? @develohpanda @darrenjennings |
Hmm, kind of on the fence about this one. Looks like there has been a conscious decision to use strong quoting (single quotes), instead of weak quoting (double quotes), as per #29. I would be inclined to make the minimum change > only updating the Is there something smarter we can do, for example detecting the OS and deciding the type of quote to apply automatically, and allow that to be overridden with a user provided option? This way shell targets would work on Windows automatically, have no impact to Linux/macOS and current behavior, but allow users to override it. |
Deciding the type of quote automatically is a great idea. This should resolve the issue when using the To manage both uses (cli + lib), I would suggest adding an option in convert named We would be able to detect the platform in bin/httpsnippet and set the option automatically. |
That sounds like a reasonable approach. What is your opinion on introducing a |
@pimterry are you able to comment on what to do here? |
Ok, so just to summarize the above:
I've done a bunch of tests on a Windows 11 cmd shell vs bash on Linux (collapsed here because there's lots)
So:
That means there is no combination of quote + escaping rules that will ever produce snippets that work correctly everywhere. I think that means a A With that, we could use single quotes in bash (i.e. strong quotes, so no need to escape anything except other single quotes, super easy) and double quotes in cmd. The only leftover case is real %variable% names in snippets, which are always inescapably (!!!) replaced with the real variable value, and AFAICT there's nothing we can do about that. If we ever find a solution it'd be easy to add it though without breaking anything else A How does that sound? |
The generated curl command used single quotes but that doesn't work for Windows users that have shell. Linux supports both single and double quotes. Changing
--header=''
to--header=""
will enable windows users with curl to use it too :).The text was updated successfully, but these errors were encountered: