Skip to content

Update DAP protocol up to 1.65#827

Merged
jonahgraham merged 6 commits into
eclipse-lsp4j:mainfrom
Soarex16:main
Apr 14, 2024
Merged

Update DAP protocol up to 1.65#827
jonahgraham merged 6 commits into
eclipse-lsp4j:mainfrom
Soarex16:main

Conversation

@Soarex16

@Soarex16 Soarex16 commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@Soarex16

Soarex16 commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

@jonahgraham I combined all updates into one PR.

@jonahgraham

Copy link
Copy Markdown
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 KamasamaK and jonahgraham and removed request for jonahgraham April 9, 2024 01:05

@jonahgraham jonahgraham left a comment

Copy link
Copy Markdown
Contributor

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

Soarex16 commented Apr 9, 2024

Copy link
Copy Markdown
Contributor Author

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

@Soarex16 Soarex16 requested a review from jonahgraham April 9, 2024 10:00

@jonahgraham jonahgraham left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
@jonahgraham jonahgraham mentioned this pull request Apr 14, 2024
35 tasks
@jonahgraham

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

3 participants