-
Notifications
You must be signed in to change notification settings - Fork 431
Easier addition of injection friendly handler #671
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
795bc5f
Easier addition of injection friendly handler
Gronex e68eee4
Using command inheritance to indicate handler
Gronex 8b814cd
adding call to BindInvocationHandler in hosting setup
Gronex d3ac81f
Making HandlerType nullable
Gronex 6eb7885
Registering the commandhandler via IHostBuilder
Gronex 44036cb
Throw exception on bad arguments
Gronex 7de36cb
Propper order of checking implemented types
Gronex a3a6b2c
Testing for Argument binding
Gronex 38a2d88
Attempting to resolve handler instance using serviceProvider
Gronex 67ed665
Async hosting tests
Gronex 112ce56
nicer not null check
Gronex d47fc04
Putting testing logic into test instead of helpers
Gronex File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
183 changes: 183 additions & 0 deletions
183
src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| using System.CommandLine.Binding; | ||
| using System.CommandLine.Builder; | ||
| using System.CommandLine.Invocation; | ||
| using System.CommandLine.IO; | ||
| using System.CommandLine.Parsing; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using FluentAssertions; | ||
|
|
||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Options; | ||
| using Xunit; | ||
|
|
||
|
|
||
| namespace System.CommandLine.Hosting.Tests | ||
| { | ||
| public static class HostingHandlerTest | ||
| { | ||
|
|
||
| [Fact] | ||
| public static async Task Constructor_Injection_Injects_Service() | ||
| { | ||
| var service = new MyService(); | ||
|
|
||
| var parser = new CommandLineBuilder( | ||
| new MyCommand() | ||
| ) | ||
| .UseHost((builder) => { | ||
| builder.ConfigureServices(services => | ||
| { | ||
| services.AddTransient(x => service); | ||
| }) | ||
| .UseCommandHandler<MyCommand, MyCommand.MyHandler>(); | ||
| }) | ||
| .Build(); | ||
|
|
||
| var result = await parser.InvokeAsync(new string[] { "--int-option", "54"}); | ||
|
|
||
| service.Value.Should().Be(54); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static async Task Parameter_is_available_in_property() | ||
| { | ||
| var parser = new CommandLineBuilder(new MyCommand()) | ||
| .UseHost(host => | ||
| { | ||
| host.ConfigureServices(services => | ||
| { | ||
| services.AddTransient<MyService>(); | ||
| }) | ||
| .UseCommandHandler<MyCommand, MyCommand.MyHandler>(); | ||
| }) | ||
| .Build(); | ||
|
|
||
| var result = await parser.InvokeAsync(new string[] { "--int-option", "54"}); | ||
|
|
||
| result.Should().Be(54); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static async Task Can_have_diferent_handlers_based_on_command() | ||
| { | ||
| var root = new RootCommand(); | ||
|
|
||
| root.AddCommand(new MyCommand()); | ||
| root.AddCommand(new MyOtherCommand()); | ||
| var parser = new CommandLineBuilder(root) | ||
| .UseHost(host => | ||
| { | ||
| host.ConfigureServices(services => | ||
| { | ||
| services.AddTransient<MyService>(_ => new MyService() | ||
| { | ||
| Action = () => 100 | ||
| }); | ||
| }) | ||
| .UseCommandHandler<MyCommand, MyCommand.MyHandler>() | ||
| .UseCommandHandler<MyOtherCommand, MyOtherCommand.MyHandler>(); | ||
| }) | ||
| .Build(); | ||
|
|
||
| var result = await parser.InvokeAsync(new string[] { "mycommand", "--int-option", "54" }); | ||
|
|
||
| result.Should().Be(54); | ||
|
|
||
| result = await parser.InvokeAsync(new string[] { "myothercommand", "--int-option", "54" }); | ||
|
|
||
| result.Should().Be(100); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static async Task Can_bind_to_arguments_via_injection() | ||
| { | ||
| var service = new MyService(); | ||
| var cmd = new RootCommand(); | ||
| cmd.AddCommand(new MyOtherCommand()); | ||
| var parser = new CommandLineBuilder(cmd) | ||
| .UseHost(host => | ||
| { | ||
| host.ConfigureServices(services => | ||
| { | ||
| services.AddSingleton<MyService>(service); | ||
| }) | ||
| .UseCommandHandler<MyOtherCommand, MyOtherCommand.MyHandler>(); | ||
| }) | ||
| .Build(); | ||
|
|
||
| var result = await parser.InvokeAsync(new string[] { "myothercommand", "TEST" }); | ||
|
|
||
| service.StringValue.Should().Be("TEST"); | ||
| } | ||
|
|
||
| public class MyCommand : Command | ||
| { | ||
| public MyCommand() : base(name: "mycommand") | ||
| { | ||
| AddOption(new Option<int>("--int-option")); // or nameof(Handler.IntOption).ToKebabCase() if you don't like the string literal | ||
| } | ||
|
|
||
| public class MyHandler : ICommandHandler | ||
| { | ||
| private readonly MyService service; | ||
|
|
||
| public MyHandler(MyService service) | ||
| { | ||
| this.service = service; | ||
| } | ||
|
|
||
| public int IntOption { get; set; } // bound from option | ||
| public IConsole Console { get; set; } // bound from DI | ||
|
|
||
| public Task<int> InvokeAsync(InvocationContext context) | ||
| { | ||
| service.Value = IntOption; | ||
| return Task.FromResult(IntOption); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public class MyOtherCommand : Command | ||
| { | ||
| public MyOtherCommand() : base(name: "myothercommand") | ||
| { | ||
| AddOption(new Option<int>("--int-option")); // or nameof(Handler.IntOption).ToKebabCase() if you don't like the string literal | ||
| AddArgument(new Argument<string>("One")); | ||
| } | ||
|
|
||
| public class MyHandler : ICommandHandler | ||
| { | ||
| private readonly MyService service; | ||
|
|
||
| public MyHandler(MyService service) | ||
| { | ||
| this.service = service; | ||
| } | ||
|
|
||
| public int IntOption { get; set; } // bound from option | ||
| public IConsole Console { get; set; } // bound from DI | ||
|
|
||
| public string One { get; set; } | ||
|
|
||
| public Task<int> InvokeAsync(InvocationContext context) | ||
| { | ||
| service.Value = IntOption; | ||
| service.StringValue = One; | ||
| return Task.FromResult(service.Action?.Invoke() ?? 0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public class MyService | ||
| { | ||
| public Func<int> Action { get; set; } | ||
|
|
||
| public int Value { get; set; } | ||
|
|
||
| public string StringValue { get; set; } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the fact that this is an exact type match, as opposed to allowing for matching on subtypes, will be less intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure if it risks causing more confusion if a command and a more specific command is registered, then the handler for the command might end up matching with the more specific command if i am understanding the scenario correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it might also be redundant with the following line.
Is there a way to ask the container if it can create an instance of
handlerType, so we don't implement logic that's inconsistent with the container configuration?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you thinking testing if the container already has the handler type registered? Or verifying we are not parsed something the container cannot possibly new up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, the container might be configured to map
ISomeCommandtoMyCommand. The current code would block that from working because of thecommand.GetType() == commandTypecheck. So in effect we're reimplementing a type matching check event though the resolved handler might work just fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the check is there to ensure that the command is the one that the caller configured to be handeled by that commandHandler.
Is it even possible to have the command as something not inheriting from
Commandlike an interface?