-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add support for startDebugging #704
Conversation
Can one of the admins verify this patch? |
@eclipse-lsp4j-bot run tests |
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.
The build is failing with this error:
09:06:31 > Task :org.eclipse.lsp4j.debug:generateXtext FAILED
09:06:31 ERROR:ProcessEventArgumentsStartMethod cannot be resolved to a type. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 486 column : 2)
09:06:31 ERROR:StartDebuggingRequestType cannot be resolved to a type. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 4299 column : 2)
09:06:31 ERROR:The type ProcessEventArgumentsStartMethod is already defined. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 499 column : 6)
09:06:31 ERROR:The type ProgressUpdateEventArguments is already defined. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 607 column : 7)
09:06:31 ERROR:The type ProgressUpdateEventArguments is already defined. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 4251 column : 7)
09:06:31 ERROR:The type ProcessEventArgumentsStartMethod is already defined. (file:/home/jenkins/agent/workspace/lsp4j-github-pullrequests/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend line : 4274 column : 6)
09:06:31
org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend
Outdated
Show resolved
Hide resolved
4a251b7
to
0b865c6
Compare
@eclipse-lsp4j-bot run tests |
...eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/services/IDebugProtocolClient.java
Outdated
Show resolved
Hide resolved
0b865c6
to
a2ae94f
Compare
@eclipse-lsp4j-bot run tests |
build currently does not run cause auf nexus.eclipse outage |
@eclipse-lsp4j-bot run tests |
a2ae94f
to
8d6bec9
Compare
I had hoped to get to this by now, but alas too many other things to review. Still hoping that @KamasamaK may swoop in and review, but I should get to it soon if he doesn't. |
@jonahgraham no big issue; I don't have strong needs for it on short term; of course -like all reviews- the sooner the better, but as I'm also busy with other things, I don't plan to get to LSP4E side of things shortly, so this can wait without harming. |
8d6bec9
to
2f525cf
Compare
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 have made a bunch of changes, each one should be explained with a review comment.
} | ||
|
||
/** | ||
* Arguments for <code>startDebugging</code> reverse-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.
Match existing style, plus "reverse" is not relevant here.
* or `attach` request. | ||
*/ | ||
@NonNull | ||
StartDebuggingRequestType 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.
For anonymous enums we have used the full type name + field name to name it. e.g OutputEventArgumentsGroup
} | ||
|
||
@JsonRpcData | ||
class StartDebuggingResponse { |
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.
Remove this - we use Void
type when there is just an ack response. e.g. there is no ConfigurationDoneResponse
* Since 1.59 | ||
*/ | ||
@JsonRequest | ||
default CompletableFuture<StartDebuggingResponse> startDebugging(StartDebuggingRequestArguments args) { |
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.
Return Void
* of the same type as the caller. | ||
* <p> | ||
* This request should only be sent if the corresponding client capability | ||
* <code>supportsStartDebuggingRequest</code> is <code>true</code>. |
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.
Use links to be consistent with existing code.
* <p> | ||
* Since 1.59 | ||
*/ | ||
Boolean supportsStartDebuggingRequest; |
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 is in the wrong place, should be in InitializeRequestArguments
as it is a capability of the client, not of the server.
/** | ||
* Client supports the `startDebugging` request. | ||
* <p> | ||
* This is an optional property. |
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.
Mixed indentation on these lines
|
||
@JsonRpcData | ||
class StartDebuggingResponse { | ||
} |
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.
Missing newline at end of the file
@jonahgraham sure, thanks a lot! |
The rest of the updates to bring the DAP part of LSP4J up to date are in #716 |
No description provided.