-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add support for tfvars
(variable files)
#540
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, this looks like a good start! I left you some in-line comments.
I have yet to decide about the language ID to use for these files but keep it tfvars
for now - I assume it's just a minor implementation detail anyway that can be changed easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look almost ready for merge 👍🏻
Aside from my in-line comments - can you also add some E2E tests here?
- https://github.com/hashicorp/terraform-ls/blob/main/internal/langserver/handlers/complete_test.go
- https://github.com/hashicorp/terraform-ls/blob/main/internal/langserver/handlers/hover_test.go
- https://github.com/hashicorp/terraform-ls/blob/main/internal/langserver/handlers/semantic_tokens_test.go
- https://github.com/hashicorp/terraform-ls/blob/main/internal/langserver/handlers/formatting_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the last two comments this LGTM! Thank you for all your effort to get it to this point @beandrad
I will have a discussion internally about what the right language ID is for tfvars - if not tfvars
and then we can merge it.
tfvars
(variable files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some more thorough testing in VS Code as well and I just noticed two more things that we should address here:
- symbols are not reported for
tfvars
, I reckon there are some minor changes needed interraform-ls/internal/langserver/handlers/symbols.go
Lines 40 to 43 in adf5e7d
d, err := decoder.DecoderForModule(ctx, mod) if err != nil { return symbols, err } - document links handler should probably just completely ignore tfvars: https://github.com/hashicorp/terraform-ls/blob/adf5e7d22fe483c72c7209dee8ee90868738f123/internal/langserver/handlers/document_link.go - I guess it's no-op anyway, but better to be explicit about it
So that we can use the language server for "tfvars" files. Related to #50
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
So that we can use the language server for "tfvars" files.
Closes #50