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 snippet proposals loaded from templates file to language server. #2639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewL-avlq
Copy link

Adds mechanism for loading a templates.xml containing code snippets for a language into the LSP language server and enabling them to be proposed for specific context assist contexts.

context="org.eclipse.xtext.ide.tests.testlanguage.TestLanguage.TypeDeclaration"
description="Add type declaration"
name="type declaration template">type ${1:name} {
$0
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding: Does this template work in Eclipse, too? Because there I'd expect something like $cursor instead of $0

Copy link
Author

Choose a reason for hiding this comment

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

I don't believe the syntax is the same, so I wouldn't expect it to work. These templates are based on the LSP snippet syntax https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#snippet_syntax

Copy link
Member

Choose a reason for hiding this comment

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

so in xtext templates there is stuff like
${Name} ${public:Enum('Visibility')} ${event:CrossReference('Transition.event')}
i assume the first one you can easily adapt to lsp variant.
for the others i assume it needs more compilated calculation to achieve something like
https://github.com/itemis/xtext-languageserver-example/blob/dfbb365d70e1370db4d4b60bc193efa173082266/org.xtext.example.mydsl.ide/src/org/xtext/example/mydsl/ide/contentassist/MyDslIdeContentProposalProvider.java#L28

Copy link
Author

Choose a reason for hiding this comment

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

In the LSP snippet syntax, tab-stops are numbered, so ${Name} would become ${1:Name}. The others use JFace resolvers which I have assumed are not available on a server, so are not supported in this commit.

Copy link
Author

Choose a reason for hiding this comment

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

So yes, something more complicated is required to convert an enum resolver to a snippet choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I'm not really a fan of the idea of duplicating the template definitions between LSP and Eclipse. I'd rather like to see templates being reused. Would you agree that the Eclipse templates could be translated into the LSP format while reading them from the XML file?

Copy link
Author

Choose a reason for hiding this comment

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

For context, these are just the classes we have used for our project to get something working, and we thought they might be useful to you. If they go against your preferred implementation direction, then we will be fine to drop the PR.

To answer your question: Yes the xml files could be translated as they are read, but...

  • The obvious issue is that there is a performance hit to translating the xml file each time and we didn't want to take that hit when the result of the translation could be written out to a new file.
  • There are also variables that can be added to a snippet that are to be evaluated in the LSP client rather than the server - in a traditional Eclipse setup there would not be an LSP client to evaluate these.
  • Also, the syntax for a client-side-variable-with-default-value is the same as a jface-variable-with-no-argument-resolver, so a translator would need to know which case it was trying to translate.

@cdietrich
Copy link
Member

builds says

11:26:04  [ERROR] /home/jenkins/agent/workspace/xtext_PR-2639/org.eclipse.xtext.ide.tests/testlang-src/org/eclipse/xtext/ide/tests/testlanguage/ide/TestLanguageIdeContentProposalProvider.java:[43] 
11:26:04  [ERROR] 	protected String getTemplatePlugin() {
11:26:04  [ERROR] 	                 ^^^^^^^^^^^^^^^^^^^
11:26:04  [ERROR] The method getTemplatePlugin() of type TestLanguageIdeContentProposalProvider must override or implement a supertype method
11:26:04  [ERROR] 1 problem (1 error)
11:26:04  [ERROR] -> [Help 1]
11:26:04  [ERROR] 

Adds mechanism for loading a templates.xml containing code snippets for
a language into the LSP language server and enabling them to be proposed
for specific context assist contexts.

Signed-off-by: Andrew Lamb <Andrew.Lamb@avaloq.com>
@szarnekow
Copy link
Contributor

@rubenporras @andrewL-avlq Sorry about the lack of feedback here from my side. I see the build is currently red. Do you plan to fix this?

@andrewL-avlq
Copy link
Author

@szarnekow You appeared to indicate that you would not approve this PR due to the approach taken, so I did not look at why the tests are failing in the verify job environment (they pass locally for me), and we have no plan to fix it while that is still true. Are you now interested in taking the contribution?

@szarnekow
Copy link
Contributor

Ok, I thought it would be easier to review and discuss based on a green build. The fact that the proposed change does not get my strongest support does not mean that others might not benefit from this or see it differently.

@cdietrich Do you have an opinion about the proposed enhancement?

@cdietrich
Copy link
Member

in general i am ok with this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants