-
Notifications
You must be signed in to change notification settings - Fork 150
Add beta support for new features in LSP 3.18 #893
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
Conversation
jonahgraham
left a comment
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.
From my perspective, approved for 1.0.0 based on #873 (comment)
|
@jonahgraham Thanks for taking a look at this PR and approving it for 1.0.0 👍 Since this PR has become a bit stale and we still have some time left before the 1.0.0 release according to #871 (comment), I'll make sure to review the current state of the LSP 3.18 spec and update this PR if necessary. |
Renames `InlineCompletionClientCapabilities` to `InlineCompletionCapabilities` to match existing naming conventions for property types in `TextDocumentClientCapabilities`.
|
I have rebased this PR and would like to merge it in its current state, if there are no objections. Looking at the LSP spec repo history, I cannot see any changes that would affect this PR. The only relevant change is this commit, which adds some optional properties to |
|
I don't quite understand what is happening here: https://github.com/eclipse-lsp4j/lsp4j/actions/workflows/junit.yml, but it does not seem related to this PR, so I think we are good to go. |
|
I was trying to get junit publishing working - I got it working on my fork (e.g. jonahgraham#1 (comment) for a PR and https://github.com/jonahgraham/lsp4j/runs/55447887659 for main) but I need to update permissions on eclipse-lsp4j (in otterdog I think) - but I didn't come back to it yet. Sorry for the noise - I will create an issue soon if I don't resolve it. |
|
I'd like to merge it now to avoid repeated rebasing 17 commits. There have been no comments for several months since this PR was created, but if anyone would be interested in reviewing it now, please let me know ASAP. |
|
I have no objections. |
|
Merging it now to allow interested parties to test the changes before they are released in |
|
@jonahgraham As a potential follow-up to this PR, what do you think of renaming the |
|
|
@pisv I overlooked this earlier, but can you add changelog + readme entries for this at your convenience, similar to what I asked for in this other PR #856 (review) |
|
@jonahgraham Sure! I don't know why I thought we were doing this just before releasing as part of the other release activities... Probably because I'm mostly contributing to Eclipse Theia these days, and they update their changelog in this way (except for breaking API changes, which should be part of each PR that introduces such changes). Anyway, let me do it just after #856 is merged to avoid conflicts. |
|
FWIW, this PR does include breaking API changes:
I don't know if it is worth explicitly listing them in the changelog too?? One of the potential limitations of japicmp (perhaps only how we are using it)
It is also part of the releasing steps - but I would prefer to have them updated as we go so that I don't miss them and so that people consuming snapshots can get the most current info at a glance.
+1 |
Yes, this has already been addressed (as part of this PR): Lines 20 to 24 in b5a8adf
I see. Makes total sense. Thanks! |
Thanks for reminding me - I was too focussed on the top part of the Changelog that today I missed that you had done this part. |
Done in #935. |
This PR complements #886 in adding beta support for those new features in LSP 3.18 that are available at the moment. The new API elements are annotated with
@Draftand are subject to incompatible changes, including even removal, as long as they remain in the draft (beta) state.Please note that there are some breaking API changes, which are documented in the change log as follows:
TextDocumentEdit.editschanged fromList<TextEdit>toList<Either<TextEdit, SnippetTextEdit>>Diagnostic.messagechanged fromStringtoEither<String, MarkupContent>DocumentFilter.patternchanged fromStringtoEither<String, RelativePattern>NotebookDocumentFilter.patternchanged fromStringtoEither<String, RelativePattern>