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

Introduce LSP Support #904

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

esmasth
Copy link
Contributor

@esmasth esmasth commented May 10, 2024

This addresses #903

@esmasth esmasth force-pushed the esmasth/issue903 branch from 6a02bce to be114e9 Compare May 10, 2024 17:31
@esmasth esmasth marked this pull request as ready for review May 10, 2024 17:38
@mbj4668
Copy link
Owner

mbj4668 commented May 12, 2024

I would prefer if lsp was an "optional dependency", i.e., if I don't use lsp I should not have to install it in order to use pyang.

Also, please add some documentation (and update the README)

@esmasth
Copy link
Contributor Author

esmasth commented May 12, 2024

Thanks @mbj4668. I can work on productization given the encouragement. Ambition level for this PR is to more or less keep to the same functionality level over LSP as much is available over CLI.

esmasth added 9 commits June 3, 2024 23:16
This addresses mbj4668#903

Signed-off-by: Siddharth Sharma <[email protected]>
Also pruned dummy textDocument/inlineValue support

Signed-off-by: Siddharth Sharma <[email protected]>
Also add markdownlint.json configuration and github action to lint

Signed-off-by: Siddharth Sharma <[email protected]>
Signed-off-by: Siddharth Sharma <[email protected]>
Also removed hard install requirement of pygls where users are not
interested in using pyang as an LSP server.

Signed-off-by: Siddharth Sharma <[email protected]>
Added more documentation

Also introduced test cases to match manual validation

Signed-off-by: Siddharth Sharma <[email protected]>
It was hardcoded to 64 with --lint earlier

Signed-off-by: Siddharth Sharma <[email protected]>
@esmasth esmasth force-pushed the esmasth/issue903 branch from f4e96a0 to fd746c2 Compare June 4, 2024 07:51
esmasth added 2 commits June 4, 2024 15:22
Signed-off-by: Siddharth Sharma <[email protected]>
It works on the shell but not in vscode tasks

Signed-off-by: Siddharth Sharma <[email protected]>
@esmasth
Copy link
Contributor Author

esmasth commented Jun 4, 2024

@mbj4668 your comments were addressed by removing pygls installation dependency. It is still kept as a development dependency.

Documentation and tests are now added to productize the code. Request your review and trial.

esmasth added 4 commits June 5, 2024 12:07
Version updates and module name updates are handled without duplicates
or stale errors.

Signed-off-by: Siddharth Sharma <[email protected]>
LSP clients do not clear diagnostics automatically based on local
file system events, and without LSP server clearing the diagnostics
LSP clients will have a stale entry for deleted/renamed URIs.

See https://microsoft.github.io/language-server-protocol/specifications/lsp
PublishDiagnostics Notification section.

Signed-off-by: Siddharth Sharma <[email protected]>
This helps in inadvertent usage of models from pyang installation or
certain environment variables when they are not controllable.

Signed-off-by: Siddharth Sharma <[email protected]>
@marvinthepa
Copy link

I tried this usinc coc.nvim in vim, but the textDocument/didChange notification did not trigger an update of the warnings (i.e. warning messages never disappear when they are fixed).

Is this just broken or might there be a configuration option that I have missed?

@esmasth
Copy link
Contributor Author

esmasth commented Aug 21, 2024

@marvinthepa thanks a lot for trying it out.

I have progressed a lot further than this pull request, but didn't disturb the initial review here to get acceptance of the approach first. I will point you to the branch on esmasth/pyang, when I push the latest.

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.

3 participants