-
Notifications
You must be signed in to change notification settings - Fork 749
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
catch error when restarting conversation #1250
Conversation
Why don't we just land https://github.com/Microsoft/BotFramework-DirectLineJS/pull/133/files ? |
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.
Looks good except for the lack of coverage. Defect fixes need accompanying unit tests.
@cwhitten I'm not sure that change is warranted yet. We should consider the implications of that change. What I'm concerned about is that the thrown error has existed since direct line was first pushed, so I'm worried about missing something regarding why it's there. |
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.
Pull Request Test Coverage Report for Build 1921
💛 - Coveralls |
8685ac5
to
522e53a
Compare
fixes #1241 This should probably be addressed in direct line, but it's not immediately clear why the error is being thrown in the first place.
522e53a
to
3ee2551
Compare
@@ -93,6 +93,20 @@ interface EmulatorProps { | |||
url?: string; | |||
} | |||
|
|||
export function endConversation(directLine: any): void { |
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 expected to be used by other operations outside this class? I'm curious why this went from private to an exported function. My comment previously was regarding a consideration given to a private static
declaration for this function.
fixes #1241
This should probably be addressed in direct line, but it's not
immediately clear why the error is being thrown in the first place.