Skip to content

Commit 506c6db

Browse files
committed
Remove ArgumentEscaping.Escape for launch script arguments
Attempting to be "helpful" by escaping arguments proved to cause more issues than it solved. We can instead massively simplify our script launching logic (and fix yet another test that had a "maybe bug") by just launching what the user gave us. This should also be easier for the user to debug.
1 parent c123557 commit 506c6db

File tree

4 files changed

+13
-131
lines changed

4 files changed

+13
-131
lines changed

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

-40
This file was deleted.

src/PowerShellEditorServices/Utility/PSCommandExtensions.cs

+3-17
Original file line numberDiff line numberDiff line change
@@ -129,25 +129,11 @@ private static StringBuilder AddCommandText(this StringBuilder sb, Command comma
129129
return sb;
130130
}
131131

132-
public static PSCommand BuildCommandFromArguments(string command, IReadOnlyList<string> arguments)
132+
public static PSCommand BuildCommandFromArguments(string command, IEnumerable<string> arguments)
133133
{
134134
// HACK: We use AddScript instead of AddArgument/AddParameter to reuse Powershell parameter binding logic.
135-
// We quote the command parameter so that expressions can still be used in the arguments.
136-
var sb = new StringBuilder()
137-
.Append('.')
138-
.Append(' ')
139-
.Append('"')
140-
.Append(command)
141-
.Append('"');
142-
143-
foreach (string arg in arguments ?? System.Linq.Enumerable.Empty<string>())
144-
{
145-
sb
146-
.Append(' ')
147-
.Append(ArgumentEscaping.Escape(arg));
148-
}
149-
150-
return new PSCommand().AddScript(sb.ToString());
135+
string script = string.Concat(". ", command, " ", string.Join(" ", arguments ?? Array.Empty<string>()));
136+
return new PSCommand().AddScript(script);
151137
}
152138
}
153139
}

test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs

+10-5
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,16 @@ await debugService.SetCommandBreakpointsAsync(
176176
Assert.Equal("[ArrayList: 0]", var.ValueString);
177177
}
178178

179-
[Fact]
180-
public async Task DebuggerAcceptsScriptArgs()
179+
// See https://www.thomasbogholm.net/2021/06/01/convenient-member-data-sources-with-xunit/
180+
public static IEnumerable<object[]> DebuggerAcceptsScriptArgsTestData => new List<object[]>()
181+
{
182+
new object[] { new object[] { "Foo -Param2 @('Bar','Baz') -Force Extra1" } },
183+
new object[] { new object[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" } }
184+
};
185+
186+
[Theory]
187+
[MemberData(nameof(DebuggerAcceptsScriptArgsTestData))]
188+
public async Task DebuggerAcceptsScriptArgs(string[] args)
181189
{
182190
// The path is intentionally odd (some escaped chars but not all) because we are testing
183191
// the internal path escaping mechanism - it should escape certains chars ([, ] and space) but
@@ -197,9 +205,6 @@ public async Task DebuggerAcceptsScriptArgs()
197205
Assert.True(breakpoint.Verified);
198206
});
199207

200-
// TODO: This test used to also pass the args as a single string, but that doesn't seem
201-
// to work any more. Perhaps that's a bug?
202-
var args = new[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" };
203208
Task _ = ExecutePowerShellCommand(debugWithParamsFile.FilePath, args);
204209

205210
AssertDebuggerStopped(debugWithParamsFile.FilePath, 3);

test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs

-69
This file was deleted.

0 commit comments

Comments
 (0)