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

New ArgumentList constructors for Process classes #371

Closed
Artoria2e5 opened this issue Nov 27, 2019 · 8 comments · Fixed by #40661
Closed

New ArgumentList constructors for Process classes #371

Artoria2e5 opened this issue Nov 27, 2019 · 8 comments · Fixed by #40661
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Process
Milestone

Comments

@Artoria2e5
Copy link

Although the ArgumentList property has been added to System.Diagnostics.ProcessStartInfo as a way to relatively safely pass command line arguments, there is no simple, one-liner way to use this interface. This proposal seeks to add such an interface wherever a constructor or static function accepts the older string arguments initializer.

Applies to:

  • System.Diagnostics.ProcessStartInfo ("S.D.PSI")
  • System.Diagnostics.Process ("S.D.P")

Changes requested:

  • New static method S.D.P::Start(string fileName, IReadOnlyCollection<string> argumentList);
  • New constructor S.D.PSI::new(string fileName, IReadOnlyCollection<string> argumentList);

The fileName parameter retains the same semantics as in the other constructors. In line with how arguments is handled, the new argumentList parameter shall be copied to the internal ArgumentList property.

Non-goals:

  • Making anything obsolete. Custom command line parsing is still widespread on Windows, and that is a good use-case for Arguments.

(How does the contribution thing works anyway? I guess I will open an issue first since I am on my phone.)

@jkotas jkotas transferred this issue from dotnet/designs Nov 27, 2019
@danmoseley danmoseley transferred this issue from dotnet/corefx Nov 27, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Diagnostics.Process untriaged New issue has not been triaged by the area owner labels Nov 27, 2019
@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Nov 28, 2019

How does the contribution thing works anyway? I guess I will open an issue first since I am on my phone.)

New APIs should go through an API review process before it is approved to start implementing.

There is no simple, one-liner way to use this interface.

Personally I'm not a big fan of providing new APIs just to help people writing "one-liner" codes, because often they look better written out with newlines and spaces. It also could be done already like:

folded away because they're wrong 😅
new ProcessStartInfo("dotnet.exe") { ArgumentList = new Collection<string> { "build", "-c Release", "-f netcoreapp5.0" }}
// Just for comparison, using the existing (string, string) ctor:
new ProcessStartInfo("dotnet.exe", "build -c Release -f netcoreapp5.0")

Compared to the constructor version, I see little difference:

new ProcessStartInfo("dotnet.exe", new Collection<string> { "build", "-c Release", "-f netcoreapp5.0" })

With that said though, perhaps passing in multiple arguments when launching a process is a common enough scenario (e.g. ProcessStartInfo already has a (string, string) ctor with the second parameter being the Arguments of the new instance) to warrant a new constructor for it.

Maybe argumentList could just be an IEnumerable<T> which the ctor fetches all the element in order and store it in the ArgumentList property?

@Artoria2e5
Copy link
Author

Wait, the initializer list works? I thought it only worked on properties with a set; thing on it.

My request for this stemmed from PowerShell/PowerShell#1995, so it mainly is intended for these less sugary languages. Since new Process().startInfo.ArgumentList = blah… doesn't work I was thinking there's gotta be something better than just doing AddRange on it.

@stephentoub
Copy link
Member

stephentoub commented Nov 29, 2019

the initializer list works? I thought it only worked on properties with a set; thing on it.

They work for any type that implements IEnumerable<T> and that has an Add(T) method.

But yes, it only works on construction.

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Nov 29, 2019

Actually, My bad. ArgumentList is a readonly property so you wouldn't be able to assign new Collection to it.... Well, that makes this suggestion better then 🙂

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 30, 2019

@Gnbrkm41 You don't need to assign new Collection when using a collection property initializer (provided that the backing property has a value set). I.e.

new ProcessStartInfo
{
    ArgumentList = {
        "arg1",
        "arg2",
    }
}

@GSPP
Copy link

GSPP commented Dec 6, 2019

This would be nice because it would handle some escaping issues. For example, passing a path containing a space to a command-line tool.

This is a common source of bugs and I think writing a correct escaping algorithm is not entirely trivial.

@adamsitnik
Copy link
Member

I like this proposal and I think it's worth discussing it during API Review.

Since ProcessStartInfo.ArgumentsList is Collection<string> I think that we should make the new methods accept Collection<string> instead of IEnumerable<T> just to avoid copying the collection (it will force the callers to use it from the begining).

public class Process
{
    public static System.Diagnostics.Process Start(string fileName, Collection<string> arguments);
}

public class ProcessStartInfo
{
    public ProcessStartInfo(string fileName, Collection<string> arguments)
}

@adamsitnik adamsitnik self-assigned this Jun 25, 2020
@adamsitnik adamsitnik added api-ready-for-review and removed untriaged New issue has not been triaged by the area owner labels Jun 25, 2020
@adamsitnik adamsitnik added this to the Future milestone Jun 25, 2020
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review labels Jul 6, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 23, 2020

Video

  • The added constructor seems to add little value, the bigger gains would be Process.Start()
namespace System.Diagnostics
{
    public partial class Process
    {
        // Existing overloads:
        // public static Process Start(string fileName);
        // public static Process Start(string fileName, string arguments);
        // public static Process Start(string fileName, string userName, SecureString password, string domain);
        // public static Process Start(string fileName, string arguments, string userName, SecureString password, string domain);
        // public static Process Start(ProcessStartInfo startInfo);
        public static Process Start(string fileName, IEnumerable<string> arguments);
    }
}

@terrajobst terrajobst 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 Jul 23, 2020
adamsitnik added a commit to adamsitnik/runtime that referenced this issue Aug 11, 2020
@adamsitnik adamsitnik modified the milestones: Future, 5.0.0 Aug 11, 2020
adamsitnik added a commit that referenced this issue Aug 13, 2020
…rload (#40661)

* add Process.Start(string fileName, IEnumerable<string> arguments) overload, fixes #371
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants