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

Lsp Terraform #1147

Merged
merged 4 commits into from
Oct 31, 2019
Merged

Lsp Terraform #1147

merged 4 commits into from
Oct 31, 2019

Conversation

Gastove
Copy link
Contributor

@Gastove Gastove commented Oct 31, 2019

This PR brings in lsp configs for working with Terraform configuration files. This configuration is backed by terraform-lsp.

terraform-lsp is... fairly limited. I've been using this at my day job, and it's still helpful, but it's nowhere near as fully featured as, say, gopls or rls.

Also true: I cannot find a good way to auto-install terraform-lsp. It requires a fully-functional go install toolchain (a modern one, using go mod, meaning go version 1.11 or greater). I don't know if there's a typical or correct mechanism for reporting missing language servers, or if it's built in! Do let me know?

Closes #878.

This is rough, untested, and might not work at this point. Just bones.
Append returns a new list; push modifies in place.
@yyoncho yyoncho merged commit c0aad96 into emacs-lsp:master Oct 31, 2019
yyoncho added a commit that referenced this pull request Oct 31, 2019
Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

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

I accidentally clicked merge instead of posting the review but you may apply the comments afterwards:

Can you update the readme accordingly?

Also true: I cannot find a good way to auto-install terraform-lsp.

Have you checked what vscode plugin does? A rule of thump is that if vscode does not have automatically installed we cannot do it too(e. g. clangd)?

As a side note, in dap-mode project we have facilities to automatically download and extract vscode extensions. We should move them here in lsp-mode and use it for automatic installation when possible.

I don't know if there's a typical or correct mechanism for reporting missing language servers, or if it's built in!

What do you mean by "missing"?


(defun lsp-terraform--make-launch-cmd ()
(-let [base `(,lsp-terraform-server)]
(if lsp-terraform-enable-logging
Copy link
Member

Choose a reason for hiding this comment

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

use when when there is one leg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, happy to. Wanted to leave a clear place for more configurations in the future, but this total makes sense to me for now 👍

@yyoncho
Copy link
Member

yyoncho commented Oct 31, 2019

If you are familiar with docker it will be great if you can add your server to lsp-docker image and add sample project to test with. It will be useful for both lsp developers(when fixing bugs) and for users(when someone wants to evaluate a server).

@Gastove
Copy link
Contributor Author

Gastove commented Oct 31, 2019

@yyoncho I'm quite familiar with docker, as it happens; I don't have a good sample project around, but I'll try and knock one together soon. I'll open an issue for this, so it doesn't get lost. Very happy to get that taken care of pretty soon here 👍

As for the rest of your feedback:

  • I haven't checked what VS Code does, but I'll have a look at that -- good point. I'd be surprised if it had a way around this, but I've been surprised before ;P
  • Happy to update the README 👍
  • By "missing," I simply meant: if a user must manually install a given language server, and they haven't, do we have an existing way to tell them what they need to do, display instructions, etc?

@Gastove
Copy link
Contributor Author

Gastove commented Oct 31, 2019

Ahhh, shoot -- the terraform-lsp README doesn't mention or link to their releases, but they are doing normal binary releases after all. This can absolutely be automated (which is what VS Code does). I'll get that wired up.

@Gastove
Copy link
Contributor Author

Gastove commented Oct 31, 2019

@yyoncho do we have an existing pattern for how to compute and download platform-specific versions? That is: terraform-lsp doesn't appear to publish a "newest" artefact, a la fsautocomplete; instead it's building platform-specific binaries with version numbers. VS Code is handling this using a Github library called Octokit; do we have any equivalent tooling that I'm just not finding, or should I write something for us? (All the info we need is available from the Github API, it'll just take a bit of parsing.)

@yyoncho
Copy link
Member

yyoncho commented Oct 31, 2019

do we have an existing way to tell them what they need to do, display instructions, etc?

No, we don't - we expect the user to check the info in the README. IMO it makes sense to have it at least for most popular language servers.

do we have an existing pattern for how to compute and download platform-specific versions?

Something similar to what FSharp and MS Pyls do?

I will move the rest of the discussion in #506 .

@Gastove
Copy link
Contributor Author

Gastove commented Oct 31, 2019

Ah, at least for F# and fsautocomplete, the fsautocomplete team publishes a "latest" version, which isn't platform dependent. I believe MS pyls is similar, but I'm less familiar with it.

In any case, it sounds like a broader solution is needed, so for now, I'm going to document up the steps to manually install the terraform server and leave it at that until #506 is resolved.

@yyoncho
Copy link
Member

yyoncho commented Oct 31, 2019

ms python seems to be platform-dependent: https://github.com/emacs-lsp/lsp-python-ms/blob/master/lsp-python-ms.el#L152

@juliosueiras
Copy link

@Gastove I added vendor folder, and tested against go 1.10/1.11 , 1.9 doesn't work due to one of the deps uses strings.Builder which is 1.10+ only

P.S. I am working on adding more features and fixing any existing issues with it

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.

Support for Terraform?
3 participants