Combine Adapter and Bot into one class #145
Conversation
There was a problem hiding this comment.
I like the changes. The elimination of the Adapters via good-old-fashioned inheritance is nice.
This is going to be very difficult to merge, however. There have been changes (since this was written) to the TestAdapter, the Bot, MiddlewareSet, and other relevant classes.
| @@ -16,7 +16,6 @@ | |||
| <ItemGroup> | |||
| <ProjectReference Include="..\..\libraries\Microsoft.Bot.Builder.BotFramework\Microsoft.Bot.Builder.BotFramework.csproj" /> | |||
| <ProjectReference Include="..\..\libraries\Microsoft.Bot.Builder\Microsoft.Bot.Builder.csproj" /> | |||
| <ProjectReference Include="..\..\libraries\Microsoft.Bot.Connector.AspNetCore\Microsoft.Bot.Connector.AspNetCore.csproj" /> | |||
There was a problem hiding this comment.
Nice to see this going away.
| } | ||
|
|
||
| // Call any registered Middleware Components looking for SendActivity() | ||
| if (context.Responses != null && context.Responses.Any()) |
There was a problem hiding this comment.
Refer to #144
Given the current factoring (of state in middleware) this would still be an issue because of this line
| return this; | ||
| } | ||
|
|
||
| public async Task ProcessActivty(string authHeader, Activity activity, Func<IBotContext,Task> callback) |
There was a problem hiding this comment.
If we do introduce a BotController class we could more the auth into that. That way the responsibility of this layer is to tie the inbound and outbound worlds together and so host the pipeline.
(This is not something we need to change for this checkin.)
|
|
||
| namespace Microsoft.Bot.Builder | ||
| { | ||
| public abstract class BotServer |
There was a problem hiding this comment.
shouldn't the class name be "AdapterBase"?
There was a problem hiding this comment.
That would also help have a nicer name for the current "BotFrameworkBotserver".
There was a problem hiding this comment.
Yeah, BotFrameworkBotServer sucks pretty bad
|
|
||
| namespace Microsoft.Bot.Builder.BotFramework | ||
| { | ||
| public class BotFrameworkBotServer : BotServer |
There was a problem hiding this comment.
Given how central this class is going to be, I'd love to put more consideration in the name. BotFrameworkBotServer sounds really awkward. Could we think of something more concise and sticky?
| if (adapter == null) | ||
| throw new ArgumentNullException(nameof(adapter)); | ||
| } | ||
| //public static void AdapterNotNull(ActivityAdapterBase adapter) |
| @@ -12,7 +12,7 @@ namespace Microsoft.Bot.Builder | |||
| { | |||
| public interface IBotContext | |||
| { | |||
| Bot Bot { get; } | |||
| BotServer Bot { get; } | |||
There was a problem hiding this comment.
Would developers get any benefit for providing their own implementation of a BotServer? If yes, maybe we could have the IBotContext depend on a IBotServer abstraction instead of a BotServer concrete (abstract) class.
| @@ -38,13 +40,13 @@ public static class JwtTokenValidation | |||
| } | |||
|
|
|||
| // Go through the standard authentication path. | |||
| await JwtTokenValidation.ValidateAuthHeader(authHeader, credentials, activity.ServiceUrl, httpClient); | |||
| await JwtTokenValidation.ValidateAuthHeader(authHeader, credentials, activity.ServiceUrl, httpClient ?? _httpClient); | |||
| @@ -22,27 +22,23 @@ namespace AlarmBot.Controllers | |||
| [Route("api/[controller]")] | |||
| public class MessagesController : Controller | |||
| { | |||
| public static BotFrameworkAdapter activityAdapter = null; | |||
| public static Bot bot = null; | |||
| public static BotFrameworkBotServer botServer = null; | |||
There was a problem hiding this comment.
Do we really need a public static member? For a different check in, might be worth revisiting the need for this pattern
There was a problem hiding this comment.
Who accesses this static member from outside of the controller?
There was a problem hiding this comment.
You want it to be a singleton, the controller is created on every request.
…are ConsoleAdapter, TestAdaapter and BotFrameworkAdapter
| @@ -311,6 +311,9 @@ public TestFlow AssertReply(Action<IActivity> validateActivity, string descripti | |||
| // NOTE: See details code in above method. | |||
| task.Wait(); | |||
|
|
|||
| if (System.Diagnostics.Debugger.IsAttached) | |||
| @@ -15,13 +15,14 @@ public class QnAMaker | |||
| public const string APIManagementHeader = "Ocp-Apim-Subscription-Key"; | |||
| public const string JsonMimeType = "application/json"; | |||
|
|
|||
| private static HttpClient g_httpClient = new HttpClient(); | |||
There was a problem hiding this comment.
why not create an instance level HttpClient when not specified rather than a global static one that is to be shared across instances of your QnAMaker claass?
| @@ -49,7 +49,7 @@ public new BotFrameworkAdapter Use(Middleware.IMiddleware middleware) | |||
|
|
|||
| protected async override Task SendActivityImplementation(IBotContext context, IActivity activity) | |||
| { | |||
| if (activity.Type == "delay") | |||
| if (activity.Type == ActivityTypesEx.Delay) | |||
There was a problem hiding this comment.
Why "Ex"? ActivityTypes would be nicer probably
[Botkit] Update README for SlackAdapter sample
BotBase