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.

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

[API proposal]: add ProcessStartInfo constructor that accepts IEnumerable<string> arguments. #66450

Closed
tmds opened this issue Mar 10, 2022 · 6 comments · Fixed by #86346
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Process good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@tmds
Copy link
Member

tmds commented Mar 10, 2022

Background and motivation

Starting a process should be easy when you have an IEnumerable<string> with arguments.

This is true if you can use the static Process.Start overload that accepts an IEnumerable.

If you need to use ProcessStartInfo to make additional configuration, like redirecting the streams, there is no way to pass in the IEnumerable<string>.

Process.ArgumentList is a Collection<string>. Because it doesn't have an AddRange, the user needs to add a loop to add the arguments one-by-one.

API Proposal

A constructor can be added to ProcessStartInfo.

 ProcessStartInfo(string fileName, IEnumerable<string> arguments)

This is consistent with rest of the API of Process where IEnumerable<string> overloads were added to existing APIs that accepted string arguments.

API Usage

ProcessStartInfo psi = new("filename", GetArguments());

Alternative designs

Risks

@tmds tmds added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process labels Mar 10, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 10, 2022
@ghost
Copy link

ghost commented Mar 10, 2022

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details
 ProcessStartInfo(string fileName, IEnumerable<string> arguments)

was proposed in #371 (comment).

But the API was omitted after review: #371 (comment).

In the API review it is assumed, you can do AddRange with an existing IEnumerable.

ProcessStartInfo psi = new();
psi.ArgumentList.AddRange(args);

However Collection<T> doesn't have AddRange: #18087.

For consistency with Process.Start and improved usability it would be nice of ProcessStartInfo would also accept IEnumerable<string> arguments.

Using ProcessStartInfo is not niche. If you start command-line tools, often you want to redirect streams, and then you need a PSI.

cc @adamsitnik

Author: tmds
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics.Process

Milestone: -

@adamsitnik
Copy link
Member

@tmds I am supportive of this idea. Could you please fill the official template?

@adamsitnik adamsitnik added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 14, 2022
@tmds
Copy link
Member Author

tmds commented Mar 24, 2022

I've applied the template.

@tmds
Copy link
Member Author

tmds commented May 9, 2022

@adamsitnik is this good for api-ready-for-review ?

@adamsitnik adamsitnik added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels May 9, 2022
@adamsitnik
Copy link
Member

@tmds it's is, I've applied the label. Thanks!

@jeffhandley jeffhandley added this to the Future milestone Jul 10, 2022
@bartonjs
Copy link
Member

Looks good as proposed.

namespace System.Diagnostics
{
    public partial class ProcessStartInfo
    {
        public ProcessStartInfo(string fileName, IEnumerable<string> arguments);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 11, 2023
@adamsitnik adamsitnik added good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels May 12, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 16, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Process good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants