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

Update DAP protocol up to 1.65 #827

Merged
merged 6 commits into from Apr 14, 2024
Merged

Conversation

Soarex16
Copy link
Contributor

@Soarex16 Soarex16 commented Apr 6, 2024

No description provided.

@Soarex16
Copy link
Contributor Author

Soarex16 commented Apr 8, 2024

@jonahgraham I combined all updates into one PR.

@jonahgraham
Copy link
Contributor

Thanks - all in one PR, but with one commit per DAP version is very helpful and should make it easier for me to review.

@KamasamaK can you have a look too - is this still in your wheelhouse?

@jonahgraham jonahgraham requested review from jonahgraham and KamasamaK and removed request for jonahgraham April 9, 2024 01:05
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.

A quick look at this looks good, I want a second set of eyes to compare the implementation here to the spec.

Some housekeeping needs to be done before this can be merged.

The SCHEMA_VERSION in the protocol/services files needs updating, such as

public static final String SCHEMA_VERSION = "1.60.0";

And the supported version in the README needs updating:

* LSP4J 0.23.* _(Next release)_ → DAP 1.60.0

And an entry in the changelog is needed - single line, like

* Implemented DAP version 1.60.0

@Soarex16
Copy link
Contributor Author

Soarex16 commented Apr 9, 2024

I updated the schema version, but didn't touch other things because I don't want to interfere with the release processes.

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.

Thanks @Soarex16 - This LGTM, but I did make a few small changes. Can you confirm that I didn't mess anything up.

What I changed:

  • Added Since comment on new fields/classes
  • Add "paperwork" for changelog/readme/schema version in the java code
  • Added memoryReference to SetExpressionResponse
  • Changed some wording to match what is in the upstream spec

@Soarex16
Copy link
Contributor Author

@jonahgraham yes, I missed memoryReference in SetExpressionResponse. I checked the changelog again and everything should be fine now.

Thank you!

@jonahgraham jonahgraham merged commit 9f66dfd into eclipse-lsp4j:main Apr 14, 2024
4 checks passed
@jonahgraham jonahgraham mentioned this pull request Apr 14, 2024
35 tasks
@jonahgraham
Copy link
Contributor

Thank you @Soarex16 for the contribution.

I have merged and kicked off the build. It should end up in snapshots in a little while.

If you want a released version, please comment on #807 any constraints/timelines you have.

@KamasamaK
Copy link
Contributor

I've finally been able to review this and found a few minor issues and one big issue that I'll address in another PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants