Change IList<string> to IEnumerable<string> in Process Run/StartAndForget methods#130630
Conversation
…rget methods Fixes #130364 by updating the parameter type of the arguments parameter in Process.Run, Process.RunAsync, Process.RunAndCaptureText, Process.RunAndCaptureTextAsync, and Process.StartAndForget to use IEnumerable<string> instead of IList<string>, consistent with the existing Process.Start(string, IEnumerable<string>) overload. Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the public System.Diagnostics.Process “scenario” APIs to accept IEnumerable<string>? for command arguments instead of IList<string>?, aligning with the existing Process.Start(string, IEnumerable<string>) pattern and removing an unnecessary list requirement.
Changes:
- Updated
Process.Run,RunAsync,RunAndCaptureText,RunAndCaptureTextAsync, andStartAndForgetoverloads to takeIEnumerable<string>? arguments = null. - Simplified internal
CreateStartInfo*helpers to pass the enumerable directly tonew ProcessStartInfo(fileName, arguments)when non-null. - Updated the
ref/contract accordingly (public API surface change).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Scenarios.cs | Updates public overload signatures and internal helper signatures; uses ProcessStartInfo(string, IEnumerable<string>) when arguments are provided. |
| src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs | Updates the public reference surface for the five affected APIs to IEnumerable<string>?. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address the feedback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Scenarios.cs:68
- This overload can throw
ArgumentExceptionwhenfileNameis empty (viaArgumentException.ThrowIfNullOrEmpty(fileName)inCreateStartInfo), but the XML docs only mention the null case. Please document the empty-string exception for consistency with the otherfileNameoverloads in this file.
/// The command-line arguments to pass to the process. Pass <see langword="null"/> or an empty sequence
/// to start the process without additional arguments.
/// </param>
/// <returns>The process ID of the started process.</returns>
/// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <see langword="null"/>.</exception>
The
argumentsparameter inProcess.Run,RunAsync,RunAndCaptureText,RunAndCaptureTextAsync, andStartAndForgetwas typed asIList<string>despite the implementation only iterating (never indexing or mutating) the collection. This is inconsistent with the existingProcess.Start(string, IEnumerable<string>)overload.ref/System.Diagnostics.Process.cs— updated all five affected method signatures fromIList<string>?toIEnumerable<string>?src/System/Diagnostics/Process.Scenarios.cs— updated matching public signatures and the privateCreateStartInfo/CreateStartInfoForCapturehelpers; implementation body unchanged (already usedforeach)API approved in #130364 (comment)