Skip to content
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

Proposed protocol addition "set-next-statement" #28 #45

Closed
wants to merge 1 commit into from

Conversation

weinand
Copy link
Contributor

@weinand weinand commented Jul 18, 2016

Protocol addition "set-next-statement" (#28) contains:

  • a new optional supportsGotoTargetsRequest capability
  • a new type GotoTarget
  • a new optional gotoTargetsRequest request
  • a new optional GotoRequest request

@gregg-miskelly
Copy link
Member

LGTM

/** A GotoTarget describes a code location that can be used as a target in the 'goto' request.
The possible goto targets can be determined via the 'gotoTargets' request.
*/
export interface GotoTarget {
Copy link
Member

@gregg-miskelly gregg-miskelly Jul 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need a 'label' string to provide a disambiguation dialog if there is more than one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregg-miskelly yes, makes sense and brings it closer to the similar StepInTarget.

@jacdavis
Copy link

IMHO, I think it would be better to adopt the VS model here. Essentially, you'd just need a GotoRequest that takes the file path and line number and can handle errors.

@gregg-miskelly
Copy link
Member

@jacdavis that wouldn't work very well with trying to build a debug engine that exposes out the debug protocol - AD7 has a bunch of different SetNextStatement related methods (EnumCodeContexts, CanSetNextStament, IDebugCodeContex2.Compare, SetNextStatement) with a disambiguation dialog in between some of them. I think we can safely combine EnumCodeContexts+CanSetNextStatement in the enumeration API. But I think if we try and combine them all together will not be able to create a debug engine that can support this.

@jacdavis
Copy link

That’s right, I forgot about EnumCodeContexts. I guess we already have the scalability issue even in vs.

From: Gregg Miskelly [mailto:notifications@github.com]
Sent: Monday, July 18, 2016 10:28 AM
To: Microsoft/vscode-debugadapter-node vscode-debugadapter-node@noreply.github.com
Cc: Jackson Davis Jackson.Davis@microsoft.com; Mention mention@noreply.github.com
Subject: Re: [Microsoft/vscode-debugadapter-node] Proposed protocol addition "set-next-statement" #28 (#45)

@jacdavishttps://github.com/jacdavis that wouldn't work very well with trying to build a debug engine that exposes out the debug protocol - AD7 has a bunch of different SetNextStatement related methods (EnumCodeContexts, CanSetNextStament, IDebugCodeContex2.Compare, SetNextStatement) with a disambiguation dialog in between some of them. I think we can safely combine EnumCodeContexts+CanSetNextStatement in the enumeration API. But I think if we try and combine them all together will not be able to create a debug engine that can support this.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/45#issuecomment-233398311, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AM3W_cIcP9fgc1LU_B4Eic3G6Z3PhOpwks5qW7ejgaJpZM4JO2mq.

@weinand
Copy link
Contributor Author

weinand commented Jul 22, 2016

I've merged the PR and added a label attribute to the GotoTarget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants