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

[#98] Add SymbolTag values for access modifiers and other modifiers #2003

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

travkin79
Copy link

I suggest to add more SymbolTag values to the LSP specification as implemented in this PR. This PR should solve issue #98.

@travkin79
Copy link
Author

@microsoft-github-policy-service agree company="Solunar GmbH"

@dbaeumer
Copy link
Member

@travkin79 thanks for the PR. To get something into the specification we need an implementation in one of the clients / servers as well. This can be VS Code or another editor. See https://github.com/microsoft/language-server-protocol/blob/main/contributing.md

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Aug 14, 2024
@dbaeumer
Copy link
Member

Are you planning on working on such an implementation?

@travkin79
Copy link
Author

travkin79 commented Aug 14, 2024

Hi @dbaeumer,

To get something into the specification we need an implementation in one of the clients / servers as well.

Thank you for the hint. I didn't know that, but I'll check the contribution guidelines.

Are you planning on working on such an implementation?

I didn't plan to extend the VSCode client or server libs (yet), since I'm not familiar with TypeScript or VSCode, but I can check if I could do it. Nevertheless, my goal is using the new symbol tags in CDT LSP (C++ plug-ins for the Eclipse IDE). But this will require adapting clangd (which I'm also not familiar with) and in best case should include adapting the LSP specification. My motivation comes from this discussion.

I prepared this PR, because I hoped for more progress on this topic. I thought, making the proposals mentioned in the issue more concrete (e.g. with the PR) simplifies discussing the details. We could convert this PR to a draft if you like.

Besides, since I only added new values to an existing type (SymbolTag), do we need a language server (and client) to actually use each of the new values? I guess, the already existing SymbolTag value Deprecated is already being used in a reference implementation, doesn't it?

I suggest the following steps to proceed:

  1. Discuss and decide about my proposals contents, i.e. the new SymbolTag values.
  2. Check if we can / have to extend one of the clients / servers to use the new values.

@dbaeumer
Copy link
Member

If we only add the values to the spec, but now client is using them then it is misleading for server implementors. This is why even for enum values, tags, ... we want to make sure we have an implementation.

* Render a symbol as "virtual", e.g. in C++.
* @since 3.18
*/
export const Virtual = 15;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Const for C++ would be useful, too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reconsidered a Const tag and I think we can use the ReadOnly tag for that purpose. Having both tags, Const and ReadOnly might be confusing since they mean the same in principle. Besides, I don't think, we should use many language-specific keywords as tags in LSP specification. Instead, we should describe the concepts behind them.

@travkin79
Copy link
Author

If we only add the values to the spec, but now client is using them then it is misleading for server implementors. This is why even for enum values, tags, ... we want to make sure we have an implementation.

Hi @dbaeumer,
Would it be sufficient if we had a server implementation in clangd (for C++) and a client implementation in LSP4E and / or CDT LSP, instead of extending the VSCode libs?

@dbaeumer
Copy link
Member

Yes, an implementation in another client is sufficient as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants