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 YANG LSP support #4390

Merged
merged 2 commits into from
Apr 1, 2024
Merged

Conversation

esmasth
Copy link
Contributor

@esmasth esmasth commented Mar 23, 2024

This addresses #4355

@github-actions github-actions bot added documentation client One or more of lsp-mode language clients labels Mar 23, 2024
@jcs090218
Copy link
Member

There is a compile error:

In toplevel form:
clients/lsp-yang.el:56:2: Error: assignment to free variable ‘lsp-json-schemas’
..........................Test lsp-byte-compilation-test backtrace:


Test lsp-byte-compilation-test condition:

    (ert-test-failed
     ((should
       (byte-compile-file library))
      :form
      (byte-compile-file "/home/runner/work/lsp-mode/lsp-mode/clients/lsp-yang.el")
      :value nil))

Can you update our CHANGELOG.org too? :)

@esmasth
Copy link
Contributor Author

esmasth commented Mar 24, 2024

I'll fix both in the next patch, thanks.

@esmasth
Copy link
Contributor Author

esmasth commented Mar 27, 2024

@jcs090218 request your rereview here

:group 'lsp-yang
:package-version '(lsp-mode . "8.0.1"))

(setq lsp-json-schemas
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... where does this variable come from? 🤔 This looks a bit odd to me. Usually, we don't change the variable at the root level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from lsp-json.el in this repo. Is it better to append to the list? How does one do that for repeat alist?

Copy link
Member

Choose a reason for hiding this comment

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

I guess my question is, why is the YANG lsp client required to modify JSON language server settings? 🤔 Most language server clients are isolated, so it's a bit odd to see something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The json schema association settings could be set in init.el or .emacs for user specified associations as per variable provided by lsp-json.el. json schema associations are not always possible to encode in the json itself due to variability in editor support and how strictly json schema is written.

The association, hence, is provided via json schema handling subsystem configuration, which is the lsp-json.el in lsp-mode based emacs world as I see. The validation et al under the hood happens via vscode json language server.

Now, it should be possible for "emacs extensions" which are json based config consumers, to provide schema and association (like it is possible via vscode extensions), as an addition to a list of known associations, else each user of lsp-yang.el for example will have to make the association themselves, or there would be a central database of association configuration, neither of which scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this setting for this commit, since setq overrides any default settings. There should be a mechanism provided by lsp-json.el which allows for appending any default or further setq.

lsp-mode.el Outdated Show resolved Hide resolved
@esmasth esmasth marked this pull request as ready for review March 30, 2024 10:22
@esmasth esmasth requested a review from jcs090218 March 30, 2024 10:36
lsp-mode.el Outdated Show resolved Hide resolved
@esmasth
Copy link
Contributor Author

esmasth commented Mar 31, 2024

Thanks @jcs090218. Could you please guide me on how to resolve the failing checks? I don't know where to start.

lsp-mode.el Show resolved Hide resolved
This addresses emacs-lsp#4355

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

Thanks @jcs090218. Could you please guide me on how to resolve the failing checks? I don't know where to start.

Don't worry about the failing tests. It's irrelevant to this PR.

@esmasth
Copy link
Contributor Author

esmasth commented Apr 1, 2024

Thanks a lot @jcs090218. I have resolved the review comments hopefully, and also fixed the settings.json match pattern as a separate commit. OK to merge? I don't have permissions.

@jcs090218 jcs090218 merged commit 4e37c36 into emacs-lsp:master Apr 1, 2024
11 of 14 checks passed
@jcs090218
Copy link
Member

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants