dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.38k stars 380 forks source link

Provide a way to detect when arguments/options do not bind to the CommandHandler #1247

Open smaine opened 3 years ago

smaine commented 3 years ago

I'm very impressed with the expressiveness of the model binding infrastructure, but I'm looking for ways to detect and prevent drift between the command line options and the handler implementation type over time.

For example, I can use model binding to set up declarative association between an option named "--preserve-configuration" and property name on a complex handler type named "PreserveConfiguration". However, if I decide later to prefer the more concise form of "--preserve-config", the model binding infrastructure will simply ignore the new value and the end result is that "PreserveConfiguration" will always be false. This can lead to very subtle bugs over time as the CLI interface and underlying handler types drift apart.

I've love to able to do something like this:

CommandHandler.Create(
                async (InvocationContext context, MyLibraryHandler handler, IConsole console, CancellationToken ct) =>
                { 
                    // Throws if there are any values in the ParseResult that haven't been consumed by the model binder
                    context.ValidateStrictModelBinding();

                    MyLibraryResult result = await handler.RunAsync();
                    console.Out.Write(result.ToString());
                });

The tricky part seems to be getting the layering right between validation and invocation, as throwing an exception directly from the handler delegate like I'm implying above seems less elegant than it could be.

jonsequitur commented 3 years ago

This has been the subject of a lot of discussion, and an alternate binding mechanism (#1012) because the name-based binding, while concise, is non-obvious. The drawback to runtime validation as you're suggesting is that the performance cost is paid by your users and any exceptions thrown are visible to your users, not to you.

My approach here has been to unit test the binding directly. Here are some examples: https://github.com/dotnet/interactive/blob/main/src/dotnet-interactive.Tests/CommandLine/CommandLineParserTests.cs

smaine commented 3 years ago

In my scenario, I'm less concerned about performance because the libraries I'm wrapping have to authenticate externally to other systems and the speed of parsing does not dominate. Still, that's why I like the opt-in, pay-for-play validation so if I want to make the tradeoff for slower performance in favor of strict validation, I can.

The unit testing approach is informative, but seems like a lot of extra overhead. I'd prefer a pattern that surfaces bugs at runtime because policing unit test coverage of this stuff is very difficult across a large dev org. The less code we have to write/maintain here to slap a consistent command-line interface on our central library, the better.