You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Script arguments to debug adapter should be wrapped in quotes if they contain spaces #190
We have to be careful/smart about this. If you wrap "-Index", "1, 2, 3" with quotes you get -Index "1, 2, 3" and now we've changed the type of the argument from [int[]] to [string]. You could say that folks should do this instead "-Index", "1", "2", "3" or "-Index 1, 2, 3". The first will work but the second will totally break because SomeCommand "-Index 1, 2, 3" is a lot different than SomeCommand -Index 1, 2, 3.
I dunno on this one. Seems like it would be a bit dangerous to do this.
Another solution would be to only support a single string and not an array of strings for args. Then it should make it more obvious that individual arguments with spaces need internal quoting e.g. "-Index 1, 2, 3 -Path 'C:\program files'". The downside is that this would be a breaking change.
Yeah, you're right, maybe we should just be accepting a single string. There's not really a good reason to accept an array of strings since it complicates everything.
Maybe we can just change our schema in VS Code for now to specify string for the args field so that the user gets a warning if they're using an array. In our LaunchRequest we can change the Args property to be JToken so that either a string or an array can be passed. We'd have to do an extra step to determine if Args contains a JValue or a JArray and then act accordingly.
Forgot to should add, we'd deprecate the array support after a version or two once we have appropriate documentation showing how to do args the right way. At the time of deprecation we could even give a specific error message from the debug adapter warning the user of this.
Another somewhat lame option if we were to keep array of strings is to add more help in the scheme e.g.:
The problem with this is that it isn't very likely users would hover over the args field to get this tip. Meh, probably better to change to using just a single string.
My original thinking with array of args was that we would eventually crack the nut on AddCommand instead of AddScript (to avoid the script injection issue Lee talked about) then we would need the array of args. I'm not sure that will ever happen at this point.
Yeah... I thought about that too. Though I think it's considered "more secure" to use AddCommand so I'd like to do it at some point. We may still be able to accomplish this with a single string by using PowerShell's parser to get an AST for the string to use for passing to AddParameter/AddArgument.
Activity
rkeithhill commentedon Mar 25, 2016
We have to be careful/smart about this. If you wrap
"-Index", "1, 2, 3"with quotes you get-Index "1, 2, 3"and now we've changed the type of the argument from[int[]]to[string]. You could say that folks should do this instead"-Index", "1", "2", "3"or"-Index 1, 2, 3". The first will work but the second will totally break becauseSomeCommand "-Index 1, 2, 3"is a lot different thanSomeCommand -Index 1, 2, 3.I dunno on this one. Seems like it would be a bit dangerous to do this.
Another solution would be to only support a single string and not an array of strings for
args. Then it should make it more obvious that individual arguments with spaces need internal quoting e.g."-Index 1, 2, 3 -Path 'C:\program files'". The downside is that this would be a breaking change.daviwil commentedon Mar 25, 2016
Yeah, you're right, maybe we should just be accepting a single string. There's not really a good reason to accept an array of strings since it complicates everything.
Maybe we can just change our schema in VS Code for now to specify
stringfor theargsfield so that the user gets a warning if they're using an array. In our LaunchRequest we can change the Args property to beJTokenso that either a string or an array can be passed. We'd have to do an extra step to determine if Args contains aJValueor aJArrayand then act accordingly.daviwil commentedon Mar 25, 2016
Forgot to should add, we'd deprecate the array support after a version or two once we have appropriate documentation showing how to do args the right way. At the time of deprecation we could even give a specific error message from the debug adapter warning the user of this.
rkeithhill commentedon Mar 25, 2016
Another somewhat lame option if we were to keep array of strings is to add more help in the scheme e.g.:
The problem with this is that it isn't very likely users would hover over the
argsfield to get this tip. Meh, probably better to change to using just a single string.My original thinking with array of args was that we would eventually crack the nut on AddCommand instead of AddScript (to avoid the script injection issue Lee talked about) then we would need the array of args. I'm not sure that will ever happen at this point.
daviwil commentedon Mar 25, 2016
Yeah... I thought about that too. Though I think it's considered "more secure" to use AddCommand so I'd like to do it at some point. We may still be able to accomplish this with a single string by using PowerShell's parser to get an AST for the string to use for passing to AddParameter/AddArgument.