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

Language Server with Definitions Only #2408

Merged
merged 51 commits into from
Sep 22, 2022
Merged

Language Server with Definitions Only #2408

merged 51 commits into from
Sep 22, 2022

Conversation

wolmir
Copy link
Contributor

@wolmir wolmir commented Sep 15, 2022

Language server for data pipelines

This PR will introduce the language server module to the extension.

Background

Data Pipelines and dvc.yaml

Data Pipelines
dvc.yaml
Pipelines
Stage command
Original issue

VSCode language support

Language extensions
Available language support list from VSCode with examples

Language Servers

Introduction
Protocol Specification
The Framework to build one
Some extension samples (Look for the ones prefixed by lsp-*)

Demo

The main purpose of this PR is to add the language server scaffolding to the extension project.
However, we also provided a small Go to Definition feature in order to add some value for the user and as an example of how to work in the server module.

The feature works inside strings of dvc.yaml files, and will provide a set of location where the symbols referenced in those strings are found inside a project.

Screen.Recording.2022-09-19.at.14.32.59.mov

@wolmir
Copy link
Contributor Author

wolmir commented Sep 16, 2022

@mattseddon @julieg18 @sroy3

Can you guys skim this and compare with the original PR, please?

The definitions handler is the simplest one, and by removing the others it allowed me to skip that
DvcYaml model entirely.

Plus some other stuff could be removed, like the pythonFiles thing.
In total there were about -800 lines from the original, but I would really like your opinion here.
If they were both ready to go, would you rather merge this one or the original one?

Copy link
Contributor

@sroy3 sroy3 left a comment

Choose a reason for hiding this comment

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

This looks good to me. All my comments are in the other PR and are mostly for my curiosity, nothing major.

@mattseddon
Copy link
Member

mattseddon commented Sep 16, 2022

@mattseddon @julieg18 @sroy3

Can you guys skim this and compare with the original PR, please?

The definitions handler is the simplest one, and by removing the others it allowed me to skip that DvcYaml model entirely.

Plus some other stuff could be removed, like the pythonFiles thing. In total there were about -800 lines from the original, but I would really like your opinion here. If they were both ready to go, would you rather merge this one or the original one?

@wolmir can you please put a demo and description and open the PR that you would like to be reviewed.

@wolmir wolmir force-pushed the dvc-lsp-just-definitions branch from e02c8d9 to af81ef0 Compare September 19, 2022 17:22
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

languageServer/src/LanguageServer.ts Outdated Show resolved Hide resolved
languageServer/.prettierignore Outdated Show resolved Hide resolved
@mattseddon mattseddon marked this pull request as ready for review September 19, 2022 22:34
@mattseddon mattseddon self-requested a review as a code owner September 19, 2022 22:34
@julieg18

This comment was marked as resolved.

@mattseddon
Copy link
Member

The language server is adding definitions in Python files (e.g on re.sub):

With language server

Screen.Recording.2022-09-20.at.9.25.05.am.mov

Expected behaviour

Screen.Recording.2022-09-20.at.9.25.51.am.mov

I would also expect that trying to "Go to definition" inside of the dvc.yaml file would open give the option to open individual files (e.g step into a python file). At least I would expect that the cmd line is split into its component parts and a definition be displayed only for the symbol that is clicked on (e.g for data/features not cmd: ... data/features). It also seems like for example-get-started you cannot go to the params file definitions:

Screen.Recording.2022-09-20.at.9.29.33.am.mov

@wolmir
Copy link
Contributor Author

wolmir commented Sep 20, 2022

This provider is way too simple to add any value, and even an implementation that satisfies the constraints Matt mentioned above is going to complicate the PR again and again

Can we leave the definitions for later as well, and keep only the server skeleton then?
So we have a chance of adding a solid definitions increment?

@wolmir
Copy link
Contributor Author

wolmir commented Sep 20, 2022

This provider is way too simple to add any value, and even an implementation that satisfies the constraints Matt mentioned above is going to complicate the PR again and again

Can we leave the definitions for later as well, and keep only the server skeleton then? So we have a chance of adding a solid definitions increment?

Actually, not really, I'm already fixing it and It's not that much more complicated.

Will update here shortly.

@wolmir wolmir force-pushed the dvc-lsp-just-definitions branch from 7465318 to 55f89a1 Compare September 22, 2022 15:02
@codeclimate
Copy link

codeclimate bot commented Sep 22, 2022

Code Climate has analyzed commit 55f89a1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 91.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.9% (0.0% change).

View more on Code Climate.

@wolmir wolmir merged commit 5e21d52 into main Sep 22, 2022
@wolmir wolmir deleted the dvc-lsp-just-definitions branch September 22, 2022 15:15
@mattseddon mattseddon added the product PR that affects product label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants