-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[debug] Wait for debugger capabilities initialization before breakpoint update #11607
Conversation
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.
Hey @vakosta, thank you for your first contribution 👍
A few general remarks:
- Please keep methods/class properties open for extension by making them
protected
. - Emitters have a certain convention sorrounding their names. In your case it would be
onDidConfigureCapabilities
.
Regarding the issue at hand: While using emitters/events works fine for this issue, we can probably improve the async handling of this using the Deferred
class. It should resolve once we run updateCapabilities
and the exposed promise should be awaited at updateBreakpoints
.
@vakosta I'm having trouble to test this successfully, since the Node.js debugger doesn't support function breakpoints at all. What language/extension are you using to test this? |
@msujew Language and extension are internal commercial development in the company, where I work. Unfortunately, I can't name them, because they are under NDA. |
@msujew as an author of another debug adapter (https://marketplace.visualstudio.com/items?itemName=EvilBeaver.oscript-debug) I think this is a good fix and capabilities now should work better. What else should be done to make this PR merged? |
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.
Alright, while I can't really test the changes for the specific use case (function breakpoints), I can confirm that nothing else regressed with these changes. As the change looks reasonable, I'll approve this.
@vince-fugnitto Do you think it's alright to include this change in the upcoming release or should we merge it shortly afterwards to catch potential issues?
@msujew I'd wait for right after the release since we didn't get the opportunity to test the actual functionality of the change as no plugin was provided. |
@vince-fugnitto Alright, sounds good to me 👍 |
What it does
Fixes: #11606
Fix an issue where debugger doesn't send breakpoints on first run.
How to test
Review checklist
Reminder for reviewers