-
Notifications
You must be signed in to change notification settings - Fork 3
Added Microsoft.Extensions.Hosting support #18
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
base: master
Are you sure you want to change the base?
Conversation
It's now possible to inject scoped services in the module constructor. It's also possible to make a custom parameter provider with IParameterProvider.
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.
Lets some questions and remarks
|
||
public CommandTree Create(Assembly assembly) | ||
|
||
public IServiceCollection Services { get; } |
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.
Why are these public now? they shouldn't be used externally.
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.
So you can easily add services when you make an extension method, e.g.:
public static class CommandExtensions
{
public static CommandTreeBuilder AddWarningsModule(this CommandTreeBuilder builder)
{
builder.Services.AddScoped<IWarningService, WarningService>();
builder.AddAssembly(typeof(CommandExtensions).Assembly);
return builder;
}
}
Now you can call the extension method AddWarningsModule
and all the required services are added:
services.AddCommands(builder =>
{
builder.AddWarningsModule();
});
|
||
namespace Miki.Framework.Commands | ||
{ | ||
internal class CommandTreeCompiler |
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.
Can we perhaps have any test cases for this?
@@ -16,15 +16,15 @@ public abstract class Node | |||
public IReadOnlyCollection<Attribute> Attributes => Type.GetCustomAttributes<Attribute>(false) | |||
.ToList(); | |||
|
|||
private MemberInfo Type { get; } | |||
public Type Type { get; } |
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.
What's the reasoning for this?
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.
This was needed when the delegate is being compiled, I need the parent type when creating the NodeExecutable:
} | ||
catch (Exception e) | ||
{ | ||
Log.Error(e); |
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.
Is this before the pipeline?
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.
No, this is after the pipeline. This is called when an exception occurred in the middelware.
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.
Can we add an event then when an exception occurs to fetch the current context state and exception?
@@ -9,6 +12,11 @@ | |||
/// </summary> | |||
public interface IContext | |||
{ | |||
/// <summary> | |||
/// The message received from discord. |
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 message recieved from Discord" is deceiving, it will change into more providers in the future.
throw new ArgumentNullException(nameof(handler)); | ||
} | ||
|
||
app.Use(_ => handler); |
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.
nani?
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 extension method .Run
is being used at the end of the pipeline, so you don't need the next middleware. This is the same as ASP.NET Core.
/// <returns></returns> | ||
public static IBotApplicationBuilder UseWhen(this IBotApplicationBuilder app, Predicate predicate, Action<IBotApplicationBuilder> configuration) | ||
{ | ||
if (app == null) |
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.
App can't be null here right? Either way, you can use Required<IBotApplicationBuilder>
to automate the process.
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.
App can't be null here right?
Probably not, this is how Microsoft implemented UseWhen
as well but I can remove this.
Either way, you can use Required to automate the process.
Required<IBotApplicationBuilder>
is not possible because it's an extension method. It would register the extension method to Required<IBotApplicationBuilder>
instead of IBotApplicationBuilder
.
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.
Fair enough, but the other params could have it set to required.
public static IServiceCollection AddCacheClient<T>(this IServiceCollection services) | ||
where T : class, IExtendedCacheClient | ||
{ | ||
services.AddSingleton<ICacheClient>(provider => provider.GetService<IExtendedCacheClient>()); |
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.
How does this work?
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.
IExtendedCacheClient
extends ICacheClient
. This registration redirects ICacheClient
to IExtendedCacheClient
, you could register it two times but that means you have (for example) two Redis connections open.
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.
That doesn't make sense, we already have connection pooling.
This PR adds Microsoft.Extensions.Hosting support to Miki.Framework. This makes it way easier to set-up a new bot in a console application or even a ASP.NET Core application.
Middlewares
This PR introduces
IBotApplicationBuilder
which is equal to IApplicationBuilder in ASP.Net Core.You can define middlewares in
ConfigureBot
:Command pipeline
To use a command stage in the new middleware, you can use
app.UseStage<ClassName>()
.To use the
CommandPipelineBuilder
extensions, you can useapp.UsePipeline
:Example bot