-
Notifications
You must be signed in to change notification settings - Fork 341
Command line parser #280
Command line parser #280
Conversation
The purpose of this library is to make command line tools first class by
providing a command line parser.
* Designed for cross-platform usage
* Lightweight with minimal configuration
* Optional but built-in support for help, validation, and response files
* Support for multiple commands, like version control tools
Sample code:
static class Program
{
static void Main(string[] args)
{
var addressee = "world";
ArgumentSyntax.Parse(args, syntax =>
{
syntax.DefineOption("n|name", ref addressee, "The addressee to greet");
});
Console.WriteLine("Hello {0}!", addressee);
}
}
|
(to be clear that's not a full blown review, just a CR with a quick round of input) |
|
Why create a new one when there's an OSS library out there that does everything already and is battle tested? https://github.com/gsscoder/commandline |
|
Brilliant start! I'm not too hot on this: That would result in compile-time validation instead of runtime validation if I made a typo, e.g. The braces are superfluous, so I'd change it to this: Also notice that I used a fluent interface there as this could easily end up being overload hell. I'm wondering if there isn't a more '.Net way' to do this, some inspiration (doesn't compile): Edit: caught up a bit on Twitter. Regarding 'commands': Edit: note that with the Edit: Another concern, there needs to be an imperative approach. E.g. If I had a plugin system for commands, how would I add them and their args? It might be better to use an instance so that I could pass this to plugins/whatever: Edit: Just noticed the readme. I much prefer your enum approach, edited the above example. |
|
I'd like to echo what @FransBouma said - why not iterate on an existing open source command line parser (if need be) like https://github.com/gsscoder/commandline |
The reasons are outlined here. I've discussed it with Frans on Twitter as well. In general, we prefer to avoid NIH and contribute to existing libraries. There are plenty of examples, like JSON.NET and LibGit2Sharp. However, you can't expect to go to someone's library and fundamentally change how it feels. In the end, that's essentially creating a new library and hence is defeating the purpose of contributing to an existing library. In particular, |
|
Thanks a bunch for the detailed feedback! I'll take a closer look later. |
|
@terrajobst thank you for the explanation - that certainly makes sense. |
|
@terrajobst forgive my ignorance. why is the use f reflection in |
|
Just an idea, commands could use nesting rather than ordering to define their options which would be nicer for @jcdickinson's extensibility story. var command = Commands.None;
var prune = false;
var message = string.Empty;
var amend = false;
var common = string.Empty;
ArgumentSyntax.Parse(
s => s.DefineOption("c", "common", ref common, "Some common/shared option."),
s => s.DefineCommand("pull", () => command = Commands.Pull, "Pulls down commits",
c => c.DefineOption("p", "prune", ref prune, "The password to use").Default(false)),
s => s.DefineCommand("commit", () => command = Commands.Commit, "Commits changes",
c => c.DefineOption("m", "message", ref message, "The commit message").Required(),
c => c.DefineOption("amend", ref amend, "Ammend the last commit").Default(false),
c => somePlugin.DefineAdditionalCommitOptions(c)),
s => somePlugin.DefineOptions(s)
); |
|
@JakeGinnivan got me thinking, what about lifting the XLinq pattern? Not sure if this is any better, just adding ideas into the mixing bowl. |
We want to be able to compile a console application with minimal dependencies. Imagine a world where we statically link the runtime and the used framework, like we do with .NET Native on UWP. If your code doesn't require reflection, you should be able to produce binary that doesn't have to link in reflection. If something as basic as command line parsing coerces you into taking a dependency on reflection, then you've two options:
Our goal is to support folks building minimal apps. So we don't want to leverage reflection. |
Supporting slashes seems problematic for cross-platform usage. When running on non-Windows platform, we'd have to disallow it anyways in order to avoid parsing issues. On Windows there is already enough prior-art to justify dropping support for it. For instance, PowerShell doesn't slashes either. Also, Windows users have started to use many ports of Unix tools, such as Git, that also don't support slashes. The only downside of removing support for slashes is that existing tools that support slashes can't start to depend on this library. Until we've anybody asking for it, it seems better to start clean.
c04d847 to
6bea801
Compare
Unix has a strong tradition for scripting. In order to make it easier
to forward arguments to scripts, it's common practice to allow options
to appear more than once. The semantics are that the last one wins. So
this:
$ tool -a this -b -a that
should be equivalent to
$ tool -b -a that
|
@terrajobst thanks for the clarification. |
|
I'm for anything that can eliminate those |
I can see that. But both, single name and multiple names is common. So that would require always offering two methods, one with a single name or one with two names. Also, the API currently supports more than two names. In order to support that I'd have to either make it a plain array (yuck!) or make the names the last parameter so it can be marked as
That seems over the top to me. There are many examples where the BCL takes a lamda. We don't provide overloads that take multiple lambdas just so that you can eliminate the braces.
That's an interesting approach. The way I've envisioned is slightly different. The Guid v = Guid.Empty;
s.DefineOption("g", ref g, Guid.Parse, "Some guid");Of course, it's easy for consumers to define extension methods. For cases where you want to provide validation on top, for example, that the string is valid file name, you could provide an extension method like this: string fileName = null;
s.DefineOptionInputFile("file", ref fileName, "Some input file");The benefit of your approach is that it's at the end and doesn't obstruct the mainflow. I like! As far as validation of mutual exclusivity or occurrences go, it might be easier to allow the caller to specify a pattern like this: string fileName;
string databaseHost;
string databaseName;
s.DefineOption("f|file", ref fileName, "The data file to use");
s.DefineOption("h|dbHost", ref databaseHost, "The database host to use");
s.DefineOption("n|dbName", ref databaseName, "The database name to use");
s.DefineSyntax("-f | -h [-n]");This would mean that you can either specify a file or a database. If you specify a database, then you can also specify its name but you don't have to. Extension methods wouldn't quite work for that.
I assume the goal would be to avoid magic strings? That's simple with C# 6: string pass;
ArgumentSyntax.Parse(args, s =>
{
s.DefineOption(nameof(pass), ref pass, "The password");
});However, I'm quite certain that we don't want to add a dependency on
It seem the biggest issue folks have with commands is that it shows that this proposal is order sensitive. The point I was trying to make on Twitter that in order to support So ignoring commands, this order is invalid: bool o;
string p;
s.DefineParameter("p", ref p, "parameter p")
s.DefineOption("o", ref o, "option o")The reason it's invalid is because for this input: it's unclear if However, the developer doesn't have to worry too much about getting the order right. The API validates that things are called in the correct order and throws
There are two options:
The first is supported by the fact that you don't have to use a The second approach is actually the easiest and that's probably what I would do. It would look like this: static class Program
{
static void Main(string[] args)
{
// Getting commands. If you want, this could be call to an IoC
// container.
var commands =
{
new ImportCommand();
new ExportCommand();
};
// Ask each command to define itself:
var result = ArgumentSyntax.Parse(args, syntax =>
{
foreach (var c in commands)
c.Define(syntax);
});
// Get the command and execute it.
var command = (Command)result.ActiveCommand.Value;
command.Execute();
}
abstract class Command
{
public abstract void Define(ArgumentSyntax syntax);
public abstract void Execute();
}
class ImportCommand : Command
{
string _keepExisting;
string _fileName;
public override void Define(ArgumentSyntax syntax)
{
syntax.DefineCommand("import", this, "Import rows");
syntax.DefineOption("k|keep-existing", ref _keepExisting, "Keep existing rows");
syntax.DefineParameter("path", ref _fileName, "File to import");
}
public void Execute()
{
// ...
}
}
class ExportCommand : Command
{
string _fileName;
public override void Define(ArgumentSyntax syntax)
{
syntax.DefineCommand("export", this, "Export rows");
syntax.DefineParameter("path", ref _fileName, "File to export to");
}
public void Execute()
{
// ...
}
}
}Thoughts? |
What about: Argument<bool> useForce = null;
Argument<string> fileName = null;
ArgumentSyntax.Parse(args, s =>
{
useForce = s.DefineOption("f|force", false, "Use force");
fileName = s.DefineParameter("file", string.Empty, "The file name to delete");
if (!fileName.IsSpecifed)
s.Report("need a file name");
});
if (useForce.Value)
Console.WriteLine("Deleting {0} with force", fileName.Value);
else
Console.WriteLine("Deleting {0}", fileName.Value); |
|
At some point we should do an API review of this component and also add solid API docs, but for now it looks good to me. |
|
I'm having issues loading the changes (size versus browser stability). Does this change support parameter trees? It's very natural to only have options be exposed if others preceded them. |
|
Currently that relies on manual validation. Do you have an approach in mind? |
|
@terrajobst thanks for the really detailed response.
I did an 80/20 split here. 80% will only ever need a maximum of two names, but, the remaining 20% could use one of the fluent methods to add more alternatives. Otherwise, I'm now in complete agreement - looking forward to this hitting the BCL! |
|
@terrajobst I started (a few months ago) and never finished a param parser based on consuming arg nomenclature. For example, you could represent The conversion was bi-directional, and it supported the idea of trees (as you can see above). The benefit of supporting trees, or exclusive groups of options is that a single parser setup can be used to define the entire input expectations of an application. A nice side benefit of the way I designed it was that docs <==> code. :-) All that said, this PR looks very nice - thank you! |
|
@whoisj - that looks pretty cool - I like that structure! var syntax = new SyntaxBuilder();
syntax.HasVerb("pull")
.HasOptional((pullOptions) =>
pullOptions.HasSwitch("all")
.HasExclusive((mergeOptions) =>
mergeOptions.HasSwitch("ff-only")
.HasSwitch("no-ff")
.HasSwitch("rebase", 'r'))
.HasSwitch("force", 'f')
.HasSwitch("prune", 'p')
.HasExclusive((tagsOptions) =>
tagsOptions.HasSwitch("tags", 't')
.HasSwitch("no-tags"))
.HasSwitch("verbose", 'v'))
.HasOptional((remoteOptions) =>
remoteOptions.HasLiteral("remote")
.HasOptional((refSpecOptions) =>
refSpecOptions.HasLiteral("refspec")));My fluent API attempt seems a little more complicated thanks to the lambdas than your original and doesn't seem any more concise.. so probably not worth it all things considered haha :) |
|
@FransBouma and @terrajobst as a victim of the gsscoder battle, I propose you add a test case that fails in gsscoder - assign the same value to two different options, eg |
The purpose of this library is to make command line tools first class by providing a command line parser.
Sample code:
Usage:
More details in the README.md.
Edit: Feedback
Considered by decided against:
Remove the notion of shared options and instead make options defined before commands applicable if, and only if, no command was specified.Rename 'command' to 'verb'