Surfacing new Exception DAP in UI #22948
Conversation
|
@michelkaporin, |
| return session.exceptionInfo({ threadId: this.threadId }); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
You can not just return null when a promise is expected, you should
return TPromise.as(null)
| @@ -77,6 +77,7 @@ export interface IExpression extends ITreeElement, IExpressionContainer { | |||
|
|
|||
| @@ -323,6 +323,10 @@ export class RawDebugSession extends v8.V8Protocol implements debug.ISession { | |||
| return this.send('stackTrace', args); | |||
| @@ -120,6 +120,15 @@ export class MockSession implements debug.ISession { | |||
| }); | |||
| conditionMessage = nls.localize('userUnhandledException', 'User-unhandled exception has occurred.'); | ||
| break; | ||
| default: | ||
| conditionMessage = ''; |
There was a problem hiding this comment.
I would prefer if the default case had the same Exception occurred message as the case when there is no exceptionInfo
| break; | ||
| } | ||
|
|
||
| title.textContent = `${conditionMessage} ${this.exceptionInfo.body.description}`; |
There was a problem hiding this comment.
You are missing a colon here between the condition message and the description
There was a problem hiding this comment.
I remember we agreed on putting a colon there, but I did not put it explicitly and used a dot on the conditionMessage instead, because description itself contains a colon after the exception type, so that it ends up as Unhandled exception has occurred: Error: test message which looks ugly with two colons in my opinion.
So currently it is printed like that:
Unhandled exception has occurred. Error: test message
Although I can use quotes on the description to make it look better with two colons:
Unhandled exception has occurred: 'Error: test message'
What do you think is better?
There was a problem hiding this comment.
That's how it looks now:
| * Returns exception info promise if the exception was thrown and the debug adapter supports 'exceptionInfo' request, otherwise null | ||
| */ | ||
| public get exceptionInfo(): TPromise<DebugProtocol.ExceptionInfoResponse> { | ||
| const session = this.process.session; |
There was a problem hiding this comment.
I would love if the exception widget always calls this method.
And all the logic is in this method, meaning that if we are not stopped we return null, if the session does not supportExceptionInfoRequest we also return something
| * Returns exception info promise if the exception was thrown and the debug adapter supports 'exceptionInfo' request, otherwise null | ||
| */ | ||
| public get exceptionInfo(): TPromise<DebugProtocol.ExceptionInfoResponse> { | ||
| const session = this.process.session; |
There was a problem hiding this comment.
We might need to introduce an interface on our side as a pair of adapter protocol exception interface - to get rid of the body and to handle the case when the session does not support exception request
…ogic to the debug model.
|
Cheers @isidorn, I've amended the code per your comments. |
| */ | ||
| public get exceptionInfo(): TPromise<DebugProtocol.ExceptionInfoResponse> { | ||
| const session = this.process.session; | ||
| if (session.capabilities.supportsExceptionInfoRequest && this.stoppedDetails && this.stoppedDetails.reason === 'exception') { |
There was a problem hiding this comment.
I do not like this check for this.stoppedDetails and this.stoppedDetails.reason === 'exception' since now we do it two times. Once before calling this method and once in the method.
We should do it in only one of those places - whichever feels better to you.
You just have to figure out what is the contract of this method and when should it be calle
|
@michelkaporin great job, merging in 🍻 |
Added support for querying the debug adapter for the exception info.
Implements #22078