Handler name-binding is not documented enough

[Sejsel]

The fact that CommandHandler.Create lambda parameter names have to match option names does not seem to be documented enough.

The only mention on the wiki seems to be here, which is next to a code example that does not even use a lambda function, so it may not be obvious on the first glance. There seems to be a lot of issues where people have run into it (#835, #782, #526, #628, ...), including me right now. I am still not even convinced this is a good way to design this API. It also seems like there are not enough errors thrown if names/types are mismatched (there are some open issues relating to this).

One thing that I believe would help a lot is explicitly mentioning this behavior in the Your first app with System.CommandLine sample.

Perhaps updating it like this would be enough:

Before

    rootCommand.Description = "My sample app";

    rootCommand.Handler = CommandHandler.Create<int, bool, FileInfo>((intOption, boolOption, fileOption) =>
    {
        Console.WriteLine($"The value for --int-option is: {intOption}");
        Console.WriteLine($"The value for --bool-option is: {boolOption}");
        Console.WriteLine($"The value for --file-option is: {fileOption?.FullName ?? "null"}");
    });

After

    rootCommand.Description = "My sample app";
    
    // Note that the parameters of the handler method are matched according to the names of the options
    rootCommand.Handler = CommandHandler.Create<int, bool, FileInfo>((intOption, boolOption, fileOption) =>
    {
        Console.WriteLine($"The value for --int-option is: {intOption}");
        Console.WriteLine($"The value for --bool-option is: {boolOption}");
        Console.WriteLine($"The value for --file-option is: {fileOption?.FullName ?? "null"}");
    });

[jonsequitur]

Thanks for the suggestion. I've updated the wiki.

This is by far the most common usability problem we've seen in the API. Since the parameter list isn't representable with an interface, we followed the example of ASP.NET MVC model binding, which is similarly dynamic and name-based. Lack of clearer documentation might be part of the problem here.

We also have ModelBinder.EnforceExplicitBinding:

command-line-api/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs

Lines 520 to 562 in 9d494eb

	[Fact]
	public void Explicit_model_binder_binds_only_to_configured_properties()
	{
	var intOption = new Option("--int-property") { Argument = new Argument<int>() };
	var stringOption = new Option("--string-property") { Argument = new Argument<string>() };
	var parser = new Parser(intOption, stringOption);
	var bindingContext = new BindingContext(parser.Parse("--int-property 42 --string-property Hello"));
	var binder = new ModelBinder<ClassWithMultiLetterSetters>
	{
	EnforceExplicitBinding = true
	};
	binder.BindMemberFromValue(obj => obj.IntOption, intOption);
	var instance = binder.CreateInstance(bindingContext) as ClassWithMultiLetterSetters;
	instance.Should().NotBeNull();
	instance.IntOption.Should().Be(42);
	instance.StringOption.Should().BeNull();
	}
	[Fact]
	public void Explicit_model_binder_binds_only_to_configured_ctor_parameters()
	{
	var intOption = new Option("-a") { Argument = new Argument<int>() };
	var stringOption = new Option("-b") { Argument = new Argument<string>() };
	var parser = new Parser(intOption, stringOption);
	var ctor = typeof(ClassWithMultiLetterCtorParameters)
	.GetConstructors(BindingFlags.Public | BindingFlags.Instance)
	.First();
	var paramInfo = ctor.GetParameters().First();
	var bindingContext = new BindingContext(parser.Parse("-a 42 -b Hello"));
	var binder = new ModelBinder<ClassWithMultiLetterCtorParameters>
	{
	EnforceExplicitBinding = true
	};
	binder.BindConstructorArgumentFromValue(paramInfo, intOption);
	var instance = binder.CreateInstance(bindingContext) as ClassWithMultiLetterCtorParameters;
	instance.Should().NotBeNull();
	instance.IntOption.Should().Be(42);
	instance.StringOption.Should().Be("the default");
	}

There's also been some discussion of creating a Roslyn analyzer to help spot these kinds of mistakes.

Some of the app model work (e.g. to support attribute-based binding) is also intended to

[jonsequitur]

Thanks for the suggestion. I've updated the wiki.

This is by far the most common usability problem we've seen in the API. Since the specific handler signature isn't representable with an interface, we followed the example of ASP.NET MVC model binding, which is similarly dynamic and name-based. Lack of clearer documentation might be part of the problem here.

There's also been some discussion of creating a Roslyn analyzer to help point out these kinds of mistakes. And app model work (e.g. to support attribute-based and other contracts for binding, such as StarFruit) is intended to make this easier to wire up correctly.

Suggestions to improve the API are also welcome of course.

[jonsequitur]

I've opened #872 to discuss the Roslyn analyzer approach.