-
Notifications
You must be signed in to change notification settings - Fork 217
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
Move to Omnisharp lib 0.18.x #1376
Move to Omnisharp lib 0.18.x #1376
Conversation
options.Receiver = new DapReceiver(); | ||
options.LoggerFactory = _loggerFactory; | ||
ILogger logger = options.LoggerFactory.CreateLogger("DebugOptionsStartup"); | ||
options.WithSerializer(new DapProtocolSerializer()); |
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.
this line won't be needed for 0.18.2
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 to me, let me know if you have more questions!
test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs
Show resolved
Hide resolved
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.
Oh, hmm, it didn't post my review when I wrote it... Ok now it's posting
src/PowerShellEditorServices/Services/DebugAdapter/Handlers/BreakpointHandlers.cs
Outdated
Show resolved
Hide resolved
}; | ||
} | ||
|
||
public Task<SetExceptionBreakpointsResponse> Handle(SetExceptionBreakpointsArguments request, CancellationToken cancellationToken) |
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.
Should this method currently return successfully? What will the behaviour be if someone tries to set an exception breakpoint? Will it just silently do nothing?
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.
I've not changed the code. Just moved it to a different space. There isn't a SupportsExceptionBreakpoints
capability so we have to handle this message... but it seems like it just silently does nothing... very odd.
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.
https://microsoft.github.io/debug-adapter-protocol/specification#arrow_left-initialized-event
frontend sends a ‘setExceptionBreakpoints’ request if one or more ‘exceptionBreakpointFilters’ have been defined (or if ‘supportsConfigurationDoneRequest’ is not defined or false)
.OnInitialize(async (server, request, cancellationToken) => { | ||
var breakpointService = server.GetService<BreakpointService>(); | ||
// Clear any existing breakpoints before proceeding | ||
await breakpointService.RemoveAllBreakpointsAsync().ConfigureAwait(false); | ||
}) | ||
.OnInitialized((server, request, response, cancellationToken) => { | ||
response.SupportsConditionalBreakpoints = true; | ||
response.SupportsConfigurationDoneRequest = true; | ||
response.SupportsFunctionBreakpoints = true; | ||
response.SupportsHitConditionalBreakpoints = true; | ||
response.SupportsLogPoints = true; | ||
response.SupportsSetVariable = true; | ||
|
||
return Task.CompletedTask; | ||
}); |
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.
What's the difference between these? Which of them handles the Initialize request?
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.
I added comments in a few places to explain what we're doing here... it's a bit confusing. @david-driscoll says it's cause we're weird 😛
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.
Naming things is hard... so I just stuck with the protocol naming for the events.
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.
I need to do some more documentation on how things work, it's on my list.
// sending configuration requests | ||
_jsonRpcServer.SendNotification(EventNames.Initialized); | ||
_debugStateService.ServerStarted.SetResult(true); |
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.
Based on this, it looks like this could be a TaskCompletionSource<bool>
(that way no allocation is needed to set the value of true
)
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.
changed.
.OnInitialize(async (server, request, cancellationToken) => { | ||
var breakpointService = server.GetService<BreakpointService>(); | ||
// Clear any existing breakpoints before proceeding | ||
await breakpointService.RemoveAllBreakpointsAsync().ConfigureAwait(false); | ||
}) | ||
// The OnInitialized delegate gets run right before the server responds to the _Initialize_ request: | ||
// https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Initialize |
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.
👍
No description provided.