microsoft / vscode-debugadapter-node

Debug adapter protocol and implementation for VS Code.
Other
273 stars 79 forks source link

Add 'entry' stop reason #102

Closed dlebu closed 7 years ago

dlebu commented 7 years ago

The entry stop reason is used in the node2 debug adapter but not present in the protocol. Here is a snippet of the message sent by the debug adapter to the clients when stopping on entry:

{"seq":0,"type":"event","event":"stopped","body":{"reason":"entry","threadId":1}}

msftclas commented 7 years ago

Hi @dlebu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor. If you're full-time or an intern, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

dlebu commented 7 years ago

@roblourens This is the change we talked about yesterday. Please let me know if there is any other needed change for localizing the string, given that it appears in the UI.

roblourens commented 7 years ago

The code for handling pause reasons is this.pauseMessageLabel.text(nls.localize('debugStopped', "Paused on {0}", newTreeInput.stoppedDetails.reason));. The only one we specifically look for is 'exception', so it might be that we don't need an explicit list of reasons in the protocol.

@weinand this came up because in VS, they break when they get a message that doesn't exactly fit the protocol.

gregg-miskelly commented 7 years ago

This would be useful in VS even if it just ignored unknown reasons -- stopping on entry works differently in VS and the IDE would want to know that we are stopping for entry point.

gregg-miskelly commented 7 years ago

CC @andrewcrawley

weinand commented 7 years ago

The "reason" attribute is really a UI string (as stated in its comment) and the use of the "_enum" in the protocol is misleading. In fact the generator for Typescript ignores the _enum and generates a string attribute. So the "_enum" is used to convey some example values but not for specifying the exact value set.

Debug adapters are expected to set a localised string in the reason attribute of the StoppedEvent and the client should not interpret this attribute in any way. For this reason I don't think that we should add another _enum value to the misleading "reason" _enum (we should rather remove the _enum from the reason attribute).

Instead I suggest that we introduce another attribute for the stopped event which contains the unlocalised reason. Clients are allowed to interpret this reason. We would use an "enum" to specify the known values and we would include the "entry" value.

@isidorn @michelkaporin

andrewcrawley commented 7 years ago

I'm fine with having a separate property for this. Having an explicit set of allowed values would be great.

weinand commented 7 years ago

This request has been addressed in #105. Thanks for the PR anyway.