8000 CommandHandler.Create without name matching by jonsequitur · Pull Request #1012 · dotnet/command-line-api · GitHub
[go: up one dir, main page]
Skip to content

CommandHandler.Create without name matching #1012

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 3, 2021
Merged

CommandHandler.Create without name matching #1012

merged 3 commits into from
Aug 3, 2021

Conversation

jonsequitur
Copy link
Contributor
@jonsequitur jonsequitur commented Aug 6, 2020 •
edited
Loading

The approach I'm experimenting with here is to avoid the most common gotcha in the System.CommandLine API, which is the name-based matching of options/arguments to their corresponding parameters in a command handler.

Given:

var stringOption = new Option<string>("--the-string");
var intOption = new Option<int>("--the-int");
var filesArg = new Argument<FileInfo[]>("the-files");
var command = new RootCommand
{
    stringOption,
    intOption,
    filesArg
};

Handler with name-based matching:

command.Handler = 
    // You have to specify the types and they have to match the options/arguments:
    CommandHandler.Create<string, int, FileInfo[]>( 
        // The names must match some alias of the symbol:
        (theString, theInt, theFiles) => 
        {
            
        });

Handler with the new proposed API:

command.Handler = 
    // Types are inferred:
    CommandHandler.Create(
        stringOption, intOption, filesArg,
        // The names don't matter:
        (s, i, fs) => 
        {
            
        });

Pros:

  • No more failed binding because of naming mismatches.
  • Type inference means it's all strongly-typed without having to repeat information, and no worries about a type mismatch.

Cons / design problems to be solved:

  • No current solution for complex model binding, since parameters map one-to-one with options/arguments, meaning we need a lot more overloads. (People have asked for this anyway since the complex model binding is non-obvious.)
  • No current solution for service parameters, e.g. IConsole, ParseResult, etc.

Please discuss.

@jonsequitur jonsequitur mentioned this pull request Aug 12, 2020
Closed
@jonsequitur jonsequitur force-pushed the CommandHandler.Create-without-name-matching branch from e5c8e3a to 3157e68 Compare August 12, 2020 14:37
This was referenced Aug 12, 2020
Open
Closed
Closed
Closed
@ohadschn
Copy link

The implementation looks exactly like my workaround here so I'm definitely on board.

@brettfo brettfo changed the base branch from master to main August 19, 2020 19:53
@jonsequitur jonsequitur force-pushed the CommandHandler.Create-without-name-matching branch from 3157e68 to fa4b0ca Compare August 28, 2020 17:22
@jonsequitur jonsequitur mentioned this pull request Oct 3, 2020
Merged
@jonsequitur jonsequitur mentioned this pull request Jan 5, 2021
Closed
@jonsequitur jonsequitur force-pushed the CommandHandler.Create-without-name-matching branch 2 times, most recently from c2c0051 to 67d3c63 Compare February 24, 2021 17:46
@jonsequitur jonsequitur changed the title [WIP] CommandHandler.Create without name matching CommandHandler.Create without name matching Mar 7, 2021
@jonsequitur jonsequitur requested a review from Keboo March 7, 2021 16:37
Keboo
Keboo approved these changes Mar 8, 2021
View reviewed changes
src/System.CommandLine/Binding/IValueDescriptor.cs

public interface IValueDescriptor<T> : IValueDescriptor
{
// FIX: (IValueDescriptor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this needs to be addressed?

@jonsequitur jonsequitur mentioned this pull request Apr 5, 2021
Open
@jonsequitur jonsequitur mentioned this pull request May 28, 2021
Open
@elgonzo elgonzo mentioned this pull request Jul 8, 2021
Open
@jonsequitur jonsequitur force-pushed the CommandHandler.Create-without-name-matching branch 3 times, most recently from 42dbc8a to c642816 Compare July 10, 2021 17:57
@jonsequitur jonsequitur force-pushed the CommandHandler.Create-without-name-matching branch from c6f32c3 to 67fa744 Compare July 10, 2021 18:37
@jonsequitur jonsequitur merged commit 93adae4 into dotnet:main Aug 3, 2021
@DavidKarlas DavidKarlas mentioned this pull request Aug 19, 2021
Merged
2 tasks
@jonsequitur jonsequitur mentioned this pull request Aug 23, 2021
Closed
@rgn rgn mentioned this pull request Nov 2, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keboo Keboo Keboo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonsequitur @ohadschn @Keboo
0