Open JaneX8 opened 6 years ago
@ElleshaHackett seems like a good idea. @ahmadnassri do you have any history/justification on the single quotes?
Related: #106, https://github.com/Kong/httpsnippet/pull/29
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
/**
- Use 'strong quoting' using single quotes so that we only need
- to deal with nested single quote characters.
- http://wiki.bash-hackers.org/syntax/quoting#strong_quoting */
This utility function is used for all shell targets. I'm wondering if
doublequote
and update all shell targets 😨 doublequote
function and only use it for curl
target 🧐 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 curl
target. But, it seems this is an issue with shell targets instead of curl specifically. I'm guessing strong quoting doesn't work at all on Windows? Strong quoting does feel like the safer option, reading the docs here.
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 httpsnippet
cli but what about when the lib is used in a web-based environment (I was thinking about apiembeded)
To manage both uses (cli + lib), I would suggest adding an option in convert named platform
. We could use the values windows
, macos
, linux
We would be able to detect the platform in bin/httpsnippet and set the option automatically. For web-based use, we would let the user set the option if needed.
That sounds like a reasonable approach.
What is your opinion on introducing a quotes
option with the values single
, double
, instead of platform
? It's a little more explicit, and can automatically be set to the correct value by the CLI bin/httpsnippet.
@pimterry are you able to comment on what to do here?
Ok, so just to summarize the above:
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 quotes
option is problematic - for a big chunk of users (everybody on Windows) single quotes will always be broken, and that probably won't be obvious to them initially, and for double quotes the escaping rules are very different for the two platforms in complicated ways that will cause weird issues.
A platform
option would make this possible OTOH. I think this would be a bit better as shell
though, supporting bash
& cmd
values, because it's totally possible to run bash on Windows (Git on Windows installs a Bash terminal by default, so it's very common), and because that would leave space for supporting shells that have their own unique issues in future.
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 shell
option wouldn't help bash users who want double quotes for easier variable usage (https://github.com/Kong/httpsnippet/issues/106). That seems like a separate problem tbh - we could add an additional quotes
option for that, to switch quote types and add escaping logic for bash special characters (throwing an error if you ever try to use cmd + single-quotes). But I think that's more like a nice-to-have, I'd ignore it for now.
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 :).