-
Notifications
You must be signed in to change notification settings - Fork 148
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
Conversation
@jonahgraham I combined all updates into one PR. |
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? |
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.
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
lsp4j/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend
Line 31 in 51711e1
public static final String SCHEMA_VERSION = "1.60.0"; |
And the supported version in the README needs updating:
Line 63 in 51711e1
* LSP4J 0.23.* _(Next release)_ → DAP 1.60.0 |
And an entry in the changelog is needed - single line, like
Line 61 in 51711e1
* Implemented DAP version 1.60.0 |
I updated the schema version, but didn't touch other things because I don't want to interfere with the release processes. |
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.
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
toSetExpressionResponse
- Changed some wording to match what is in the upstream spec
@jonahgraham yes, I missed Thank you! |
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. |
No description provided.