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 deprecated boolean property to CompletionItem and SymbolInformation #332

Merged
merged 1 commit into from Apr 26, 2018

Conversation

@rcjsuen
Copy link
Contributor

commented Apr 5, 2018

I've added a new boolean deprecated property both CompletionItem and SymbolInformation.

See microsoft/language-server-protocol#139.

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

@rcjsuen thanks for the PR. Since the PR adds new properties to items that flow into both directions (form client -> server and server -> client) we need to add a client capability if the client can handle the property correctly. Otherwise a server might be surprised if for example the deprecated property is lost when a completion items flows from the client to a server in a completion item resolve call.

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@dbaeumer Thank you for the review, Dirk. I understand what you mean. Does this look okay?

export interface TextDocumentClientCapabilities {
  completion?: {
    completionItem?: {
      /**
       * Client supports the deprecated property on a completion item.
       */
      deprecatedSupport?: boolean
    }
  }
}
@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@dbaeumer And just to double-check, I don't need to add a new capability for the SymbolInformation case since it's only ever sent one-way from the client to the server, correct?

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

I tried adding a capability but the Travis CI build didn't go through. Will investigate some more...

client/src/client.ts(1306,152): error TS2322: Type '{ snippetSupport: true; commitCharactersSupport: true; documentationFormat: MarkupKind[]; depreca...' is not assignable to type '{ snippetSupport?: boolean | undefined; commitCharactersSupport?: boolean | undefined; documentat...'.
  Object literal may only specify known properties, and 'deprecatedSupport' does not exist in type '{ snippetSupport?: boolean | undefined; commitCharactersSupport?: boolean | undefined; documentat...'.
@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

As VS Code doesn't read the deprecated value, the deprecatedSupport capability should be set to false, correct?

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@rcjsuen correct.

Add deprecated boolean property to CompletionItem and SymbolInformation
Introduce a new boolean property to indicate if a CompletionItem or a
SymbolInformation is depreated.

Signed-off-by: Remy Suen <remy.suen@gmail.com>

@rcjsuen rcjsuen force-pushed the rcjsuen:deprecated branch from b6d03cd to ca04df9 Apr 17, 2018

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

@dbaeumer Thanks, Dirk. I have updated this pull request.

Since VS Code doesn't support this feature I didn't bother updating ProtocolCompletionItem based on your earlier comment of letting the property become "lost" between calls. If that was a misinterpretation on my part, I can of course update this pull request and change ProtocolCompletionItem so that it will preserve the property between textDocument/completion and completionItem/resolve requests. Just let me know, thanks!

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

@rcjsuen thanks for the PR. I merged it in. Think about it we should try to preserve the value also VS Code doesn't support deprecation. But you can do this with a separate PR.

@dbaeumer dbaeumer merged commit 498a95c into microsoft:master Apr 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@dbaeumer

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

@rcjsuen can you please update the specification as well. This is necessary so that we can release a new version.

@rcjsuen rcjsuen deleted the rcjsuen:deprecated branch Apr 26, 2018

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

Hi, @dbaeumer, thanks for merging this in. I've opened microsoft/language-server-protocol#465 for updating the spec.

Regarding your earlier comment, please confirm the following.

  1. Change VS Code side's deprecatedSupport to return true.
  2. Add a new deprecated property to ProtocolCompletionItem and tweak the code accordingly so that it will be preserved across textDocument/completion and completionItem/resolve requests.

If the above is correct I will open a new PR. If I misunderstood anything, please let me know. Thanks!

@dbaeumer

This comment has been minimized.

Copy link
Member

commented May 8, 2018

@rcjsuen regarding the questions:

  1. no we should keep false here since VS Code will not do anything with the value
  2. correct.

EDIT: actually we should return true. The fact that it is not rendered is not something the server needs to know. Interesting is that the value is correctly preserved.

@rcjsuen

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2018

Thank you, @dbaeumer. I've opened #347 based on your response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.