-
Notifications
You must be signed in to change notification settings - Fork 320
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
base: main
Are you sure you want to change the base?
Add snippet proposals loaded from templates file to language server. #2639
Conversation
context="org.eclipse.xtext.ide.tests.testlanguage.TestLanguage.TypeDeclaration" | ||
description="Add type declaration" | ||
name="type declaration template">type ${1:name} { | ||
$0 |
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.
For my understanding: Does this template work in Eclipse, too? Because there I'd expect something like $cursor instead of $0
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.
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
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.
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
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.
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.
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.
So yes, something more complicated is required to convert an enum resolver to a snippet choice.
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.
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?
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.
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.
builds says
|
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 <[email protected]>
308b894
to
df6910d
Compare
@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? |
@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? |
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? |
in general i am ok with this feature. |
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.