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

Add support for startDebugging #704

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

mickaelistria
Copy link
Contributor

No description provided.

@eclipse-lsp4j-bot
Copy link

Can one of the admins verify this patch?

@jonahgraham
Copy link
Contributor

@eclipse-lsp4j-bot run tests

Copy link
Contributor

@jonahgraham jonahgraham left a 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 

@jonahgraham
Copy link
Contributor

@eclipse-lsp4j-bot run tests

@cdietrich
Copy link
Contributor

@eclipse-lsp4j-bot run tests

@cdietrich
Copy link
Contributor

build currently does not run cause auf nexus.eclipse outage
https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/407/console

@jonahgraham
Copy link
Contributor

@eclipse-lsp4j-bot run tests

@cdietrich cdietrich added this to the 0.21.0 milestone Mar 13, 2023
@mickaelistria mickaelistria requested review from jonahgraham and removed request for KamasamaK March 14, 2023 10:30
@jonahgraham
Copy link
Contributor

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.

@mickaelistria
Copy link
Contributor Author

@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.

Copy link
Contributor

@jonahgraham jonahgraham left a 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
Copy link
Contributor

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;
Copy link
Contributor

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 {
Copy link
Contributor

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) {
Copy link
Contributor

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>.
Copy link
Contributor

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;
Copy link
Contributor

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.
Copy link
Contributor

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 {
}
Copy link
Contributor

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

@mickaelistria
Copy link
Contributor Author

@jonahgraham sure, thanks a lot!

@jonahgraham jonahgraham merged commit 3838d20 into eclipse-lsp4j:main Mar 23, 2023
@jonahgraham
Copy link
Contributor

The rest of the updates to bring the DAP part of LSP4J up to date are in #716

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.

5 participants