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

Make it possible to configure lsp-ansible as add-on? t #4053

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

mpereira
Copy link
Contributor

So that it can work with yamlls.

@github-actions github-actions bot added the client One or more of lsp-mode language clients label May 14, 2023
@@ -234,6 +241,7 @@ Pretty print the content of PARAMS."
(lsp-package-path 'ansible-language-server))
,@(cl-rest lsp-ansible-language-server-command))))
:priority 1
:add-on? lsp-ansible-add-on?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just set this to t? And remove custom variable lsp-ansible-add-on?.

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'd be ok with that. I chose to make it configurable mostly for backwards compatibility.

Copy link
Member

@jcs090218 jcs090218 May 23, 2023

Choose a reason for hiding this comment

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

Is the ansible server designed to be used with other language servers? If that's the case, we can just set it to t. If no, then the PR is ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcs090218 I force-pushed making the defcustom t by default, so that it can still be customized if necessary.

47d79d9

diff --git a/clients/lsp-ansible.el b/clients/lsp-ansible.el
index be2263bbe..931bf0d50 100644
--- a/clients/lsp-ansible.el
+++ b/clients/lsp-ansible.el
@@ -34,9 +34,11 @@
   :link '(url-link "https://github.com/ansible/ansible-language-server")
   :package-version '(lsp-mode . "8.0.1"))
 
-(defcustom lsp-ansible-add-on? nil
-  "Make the lsp client `add-on' so that it works with other language servers.
-E.g., yamlls."
+(defcustom lsp-ansible-add-on? t
+  "Make the client `add-on' so that it works with other language servers.
+`yamlls`is a common one.
+
+Enabled by default."
   :type 'boolean
   :group 'lsp-ansible
   :package-version '(lsp-mode . "8.0.1"))

@jcs090218
Copy link
Member

Should be good to merge. Thanks!

@jcs090218 jcs090218 merged commit 2dbcc2c into emacs-lsp:master Jul 31, 2023
13 of 14 checks passed
@mpereira mpereira deleted the lsp-ansible-add-on-option branch July 31, 2023 17:54
@mpereira
Copy link
Contributor Author

@jcs090218 thank you for your work on lsp-mode 🙇

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants