refactor(language service): Clean up the language service interface #831
Conversation
|
We have really not put an effort to support language service as a library, it is not something we regularly check for. Are there any tests that can be added so that this does not get broken in the future. |
|
Hey @gorkem, I'm going to be more involved with API changes to the YAML service.
IMO this isn't really related to anything we can test against. It is more related to the overall design which would have needed to been addressed in the initial design of the function. The root change of this PR is here, which is making the non-essential arguments of the core YAML Language Service optional. They are not essential to core language service functionality and most users of the API will not need the functionality they provide. Without this PR it forces complex creation of unnecessary objects to get access to the language service. I think there is some work we can do to improve this API and would be interested in contributing to it. I've seen lots of use of the language service api, so I think this is something that would be beneficial to the community. |
|
This is awesome. It is a reunion with my oldterns :) I think I understand what the PR is trying to achieve. I think there should be a test that tries to initialize the language service as you described and check if it works as expected. Otherwise, we do not regularly test the project in such usage. |
|
Just popping in to join the reunion :D |
|
Its a small world in the IDE space :) I never thought I'd have a reunion on a Github issue lol.
I see now, that makes sense. I'll add that in to this PR soon. Thanks :) |
nkomonen-amazon
force-pushed
the
jpinkney-aws/refactor-library
branch
2 times, most recently
from
4bb68cc to
f9ca463
Compare
nkomonen-amazon
force-pushed
the
jpinkney-aws/refactor-library
branch
from
f9ca463 to
fef4e02
Compare
|
@nkomonen-amazon the new test is failing for me locally and at CI too. |
Problem: - Some of the functionality currently exposed by the language service doesn't make sense when running the language service as a library rather than through the language server Solution: - Clean up the language service interface and mark connection, telemetry, and yamlSettings as optional
This adds some tests to: - Test against the minimal arguments needed for the getLanguageService() api - Test the happy path of the hover functionality to ensure hover works without any of the optional arguments. Signed-off-by: Nikolas Komonen <[email protected]>
From a previous change, when creating a hover indentations were replaced with the html entity non breaking space. The issue is that the indentation property is optional and this was not considered in that change. Due to this the markdown content would be incorrect. This change performs the markdown content modification only when the expected parameters exist. Signed-off-by: Nikolas Komonen <[email protected]>
nkomonen-amazon
force-pushed
the
jpinkney-aws/refactor-library
branch
from
414d988 to
86d0fc5
Compare
|
@gorkem I've updated with a new commit that fixes the issue |
|
@msivasubramaniaan Can you check this one? |
|
@jpinkney-aws @nkomonen-amazon damn I didn't even see you leave |
What does this PR do?
Some of the functionality currently exposed by the language service doesn't make sense when running the language service as a library rather than through the language server. This PR seeks to clean up some of those cases by marking connection, telemetry, and yamlSettings as optional
What issues does this PR fix or reference?
None
Is it tested? How?
Tested by running
npm run testand then ensuring that VSCode-YAML works with these changes