Cleaned up adapter #544
Cleaned up adapter #544
Conversation
Stevenic
commented
Oct 15, 2018
Stevenic
commented
- Updated code to use async/await
- Fixed status codes returned by processActivity().
- Added ability to pass ConversationParamters to createConversation().
- Updated code to use async/await - Fixed status codes returned by processActivity(). - Added ability to pass ConversationParamters to createConversation().
| }); | ||
| } catch (err) { | ||
| return Promise.reject(err); | ||
| public createConversation(reference: Partial<ConversationReference>, logic: (context: TurnContext) => Promise<void>): Promise<void>; |
There was a problem hiding this comment.
The C# SDK has a single method here called:
public virtual async Task CreateConversationAsync(string channelId, string serviceUrl, MicrosoftAppCredentials credentials, ConversationParameters conversationParameters, BotCallbackHandler callback, CancellationToken cancellationToken)
Neither of the overloads here seem to align with that.
Can you please remove this change - which is adding new interfaces / functionality - into a different PR?
| } catch (err) { | ||
| return Promise.reject(err); | ||
| public createConversation(reference: Partial<ConversationReference>, logic: (context: TurnContext) => Promise<void>): Promise<void>; | ||
| public createConversation(reference: Partial<ConversationReference>, params: ConversationParameters, logic: (context: TurnContext) => Promise<void>): Promise<void>; |
There was a problem hiding this comment.
Why wouldn't this overload be async as well?
There was a problem hiding this comment.
This overload is new, but doesn't have a test.
| } | ||
|
|
||
| // Initialize params | ||
| params.bot = reference.bot; |
There was a problem hiding this comment.
This logic in here seems like it should be closer to the C# code, if changes are going to be made. Wouldn't be hard to much more closly align them.
I would suggest removing this change from this PR, and have this PR just do the async/await change.
|
How about we make this separate PRs?
|
Pull Request Test Coverage Report for Build 1905
💛 - Coveralls |
