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

Separate syntax for .tfvars #48

Closed
rchl opened this issue Jun 16, 2021 · 4 comments · Fixed by #56
Closed

Separate syntax for .tfvars #48

rchl opened this issue Jun 16, 2021 · 4 comments · Fixed by #56

Comments

@rchl
Copy link
Member

rchl commented Jun 16, 2021

The .tf and .tfvars should probably use separate syntaxes. Why? Please see the related issue in the terraform-ls repo (hashicorp/terraform-ls#556)

In summary, the language server expects different language IDs to be used for each of those extensions. When same syntax is used for both then that makes it impossible to send different IDs when using the LSP package.

This could be fixed by creating a separate, small sublime-syntax file for the .tfvars files and just including original syntax from it.

@radeksimko
Copy link
Collaborator

I posted more context (a longer explanation of why) in that issue.

The grammar currently in use seems to be designed to only handle *.tf files
https://github.com/alexlouden/Terraform.tmLanguage/blob/54d8350c3c5929c921ea7561c932aa15e7d96c48/Terraform.tmLanguage#L76

However I think it would be fine as a short term solution to use that grammar as it's otherwise "close enough". We do the same thing in our VS Code extension: https://github.com/hashicorp/vscode-terraform/blob/78bfa965e43569e7645b2fede889aecddbc6aaef/package.json#L66-L76

@rchl
Copy link
Member Author

rchl commented Jun 16, 2021

I'd agree that we could implement a short term solution for now since those grammars are already assigned to handle .tfvars files [1] so it wouldn't be much of a change for the end user (just a change of the syntax name in the bottom right corner possibly).

I could prepare a PR to handle that but have one concern/question. The *.tmLanguage format is obsolete in ST3/4. The *.sublime-syntax is the new format that also supports some extra features. Personally I wouldn't care in supporting syntaxes in the tmLanguage format anymore as it's too much burden. The new format is supported from build 30xx (don't remember now exactly) so it should cover 99.9% of people.

I don't know how much decision power you have over this repo but I would just drop the tmLanguage syntax. Or at least branch it off and create separate version of the package for old ST builds.

[1] https://github.com/alexlouden/Terraform.tmLanguage/blob/54d8350c3c5929c921ea7561c932aa15e7d96c48/Terraform.sublime-syntax#L28-L31

@radeksimko
Copy link
Collaborator

I could prepare a PR to handle that but have one concern/question. The *.tmLanguage format is obsolete in ST3/4.

Would that cause any particular problems to users of ST3/4 if we keep using it? I'd have thought that with both syntaxes in place, the new is preferred for the new versions?

Personally I wouldn't care in supporting syntaxes in the tmLanguage format anymore as it's too much burden.

It is burden for sure but there are other use cases outside of Sublime Text for the "old" TextMate grammar - e.g. GitHub itself highlights HCL snippets. This repo is linked from linguist:
https://github.com/github/linguist/blob/4184bbf8066d8aa908d437af6a58b46049fc3f8b/vendor/README.md#L183

Would this block assigning these different grammars to different language IDs, or is that more of an unrelated (although valid) question that popped up as you were looking at the codebase?

@rchl
Copy link
Member Author

rchl commented Jun 16, 2021

The ST will use the sublime-syntax version indeed. The only concern I had is that if I would make a PR, someone would probably expect me to also update the tmLanguage variant and that feels like a wasted effort to me. And maybe more importantly, I don't think the the tmLanguage format supports including from other syntaxes so it would have to be a duplicate of existing file which would add even more maintanace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants