-
Notifications
You must be signed in to change notification settings - Fork 281
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
Updated Dialog Manager to work with skills #2343
Conversation
Pull Request Test Coverage Report for Build 144966
💛 - Coveralls |
@chon219, I will take a look at this today. Thanks for jumping on it! |
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.
Thanks for the PR! There's some slight style changes that should be done, and additional test coverage that needs to be implemented for DialogManager.
export function shouldSendEndOfConversationToParent(context: TurnContext, turnResult: DialogTurnResult): boolean { | ||
if (!(turnResult.status == DialogTurnStatus.complete || turnResult.status == DialogTurnStatus.cancelled)) { | ||
// The dialog is still going, don't return EoC. | ||
return false; | ||
} | ||
|
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.
@chon219, this is a combination that combines DialogManager.ShouldSendEndOfConversationToParent()
and DialogExtensions.SendEoCToParent()
, correct?
I think we should also take this change in C#. What do you think @gabog?
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.
@stevengum I think so. We have two implementations of this function in c# but it seems that we only need one.
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.
Just need a test for the snapshotTrace behavior, then this PR will be good to go.
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.
Temporarily blocking; design meeting to occur next week.
Fixes #1970
Fixes #2263
Description
Updated DialogManager to support skills
Specific Changes
Testing
Added tests for Dialog Manager to cover skills