Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

Merged
merged 9 commits into from
May 19, 2023
Merged

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f52d103
Add ProcessStartInfo constructor that accepts IEnumerable<string> arg…
jbhensley May 16, 2023
5c925c7
Add more test scenarios
jbhensley May 16, 2023
dbf7815
Simplify collection initialization
jbhensley May 16, 2023
e9c62f9
Fix API summary tags
jbhensley May 17, 2023
7623449
Simplify unit tests
jbhensley May 17, 2023
bd45642
Refactor various call sites to use new overload
jbhensley May 17, 2023
1b5e494
Throw ArgumentNullException
jbhensley May 18, 2023
2e0a9a0
Modify SuperFileCheck to use new ProcessStartInfo ctor
jbhensley May 18, 2023
356bd75
- Revert calls to new overload.
jbhensley May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ public sealed partial class ProcessStartInfo
public ProcessStartInfo() { }
public ProcessStartInfo(string fileName) { }
public ProcessStartInfo(string fileName, string arguments) { }
public ProcessStartInfo(string fileName, System.Collections.Generic.IEnumerable<string> arguments) { }
public System.Collections.ObjectModel.Collection<string> ArgumentList { get { throw null; } }
[System.Diagnostics.CodeAnalysis.AllowNullAttribute]
public string Arguments { get { throw null; } set { } }
Expand Down
8 changes: 1 addition & 7 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,13 +1330,7 @@ public static Process Start(string fileName, IEnumerable<string> arguments)
ArgumentNullException.ThrowIfNull(fileName);
ArgumentNullException.ThrowIfNull(arguments);

var startInfo = new ProcessStartInfo(fileName);
foreach (string argument in arguments)
{
startInfo.ArgumentList.Add(argument);
}

return Start(startInfo)!;
return Start(new ProcessStartInfo(fileName, arguments))!;
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}

/// <devdoc>
Expand Down
14 changes: 14 additions & 0 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ public ProcessStartInfo(string fileName, string arguments)
_arguments = arguments;
}

/// <devdoc>
/// Specifies the name of the application that is to be started, as well as a set
/// of command line arguments to pass to the application.
/// </devdoc>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devdoc is relic of the past (it's not documented anywhere, you had no chance to know about it), please use standard summary tags

Suggested change
/// <devdoc>
/// Specifies the name of the application that is to be started, as well as a set
/// of command line arguments to pass to the application.
/// </devdoc>
/// <summary>
/// Specifies the name of the application that is to be started, as well as a set
/// of command line arguments to pass to the application.
/// </summary>

public ProcessStartInfo(string fileName, IEnumerable<string> arguments)
{
_fileName = fileName;

if(arguments != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we allow for nulls in the new API?

Since the argument is not marked as nullable, the method should throw for nulls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move these checks into the new ProcessStartInfo ctor

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs

Lines 1328 to 1331 in af1171b

public static Process Start(string fileName, IEnumerable<string> arguments)
{
ArgumentNullException.ThrowIfNull(fileName);
ArgumentNullException.ThrowIfNull(arguments);

I'm a little worried about consistency. The other ctors for ProcessStartInfo are not throwing ArgumentNullException and Process.Start(string fileName, IEnumerable<string> arguments) is the only overload of that method that throws.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other ctors for ProcessStartInfo are not throwing

This is true, but they are old and we can't easily change their behavior. For the new APIs, we either allow for nulls by making an argument nullable, or we don't and throw. Since the API was not approved as nullable (no ?), we should throw. By doing that we send a clear message in the API contract: nulls are not allowed, if you have a null arguments, just don't use this API.

{
_argumentList = new Collection<string>(new List<string>(arguments));
}
}

/// <devdoc>
/// Specifies the set of command line arguments to use when starting the application.
/// </devdoc>
Expand Down
26 changes: 24 additions & 2 deletions src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,15 +1257,37 @@ public void UninitializedArgumentList()
}

[Fact]
public void InitializeWithArgumentList()
public void InitializeWithArgumentList_Add()
{
ProcessStartInfo psi = new ProcessStartInfo("filename");
psi.ArgumentList.Add("arg1");
psi.ArgumentList.Add("arg2");
psi.ArgumentList.Add(" arg3");
psi.ArgumentList.Add("arg4 ");
psi.ArgumentList.Add("arg 5");
psi.ArgumentList.Add($"arg{Environment.NewLine}6");

Assert.Equal(2, psi.ArgumentList.Count);
Assert.Equal(6, psi.ArgumentList.Count);
Assert.Equal("arg1", psi.ArgumentList[0]);
Assert.Equal("arg2", psi.ArgumentList[1]);
Assert.Equal(" arg3", psi.ArgumentList[2]);
Assert.Equal("arg4 ", psi.ArgumentList[3]);
Assert.Equal("arg 5", psi.ArgumentList[4]);
Assert.Equal($"arg{Environment.NewLine}6", psi.ArgumentList[5]);
}

[Fact]
public void InitializeWithArgumentList_Enumerable()
{
ProcessStartInfo psi = new ProcessStartInfo("filename", new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" });

Assert.Equal(6, psi.ArgumentList.Count);
Assert.Equal("arg1", psi.ArgumentList[0]);
Assert.Equal("arg2", psi.ArgumentList[1]);
jbhensley marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(" arg3", psi.ArgumentList[2]);
Assert.Equal("arg4 ", psi.ArgumentList[3]);
Assert.Equal("arg 5", psi.ArgumentList[4]);
Assert.Equal($"arg{Environment.NewLine}6", psi.ArgumentList[5]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it could be simplified

Suggested change
ProcessStartInfo psi = new ProcessStartInfo("filename", new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" });
Assert.Equal(6, psi.ArgumentList.Count);
Assert.Equal("arg1", psi.ArgumentList[0]);
Assert.Equal("arg2", psi.ArgumentList[1]);
Assert.Equal(" arg3", psi.ArgumentList[2]);
Assert.Equal("arg4 ", psi.ArgumentList[3]);
Assert.Equal("arg 5", psi.ArgumentList[4]);
Assert.Equal($"arg{Environment.NewLine}6", psi.ArgumentList[5]);
string[] args = new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" };
ProcessStartInfo psi = new ProcessStartInfo("filename", args);
Assert.Equal(args, psi.ArgumentList);

}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))] // No Notepad on Nano
Expand Down