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

Split the tfvars handling into a separate syntax file #49

Merged
merged 4 commits into from Jun 21, 2021

Conversation

rchl
Copy link
Contributor

@rchl rchl commented Jun 16, 2021

The main syntax no longer handles the .tfvars extension. Handling of
.tfvars is now done by the new syntax file which also assigns a new
source.terraform-vars scope instead of the previous source.terraform.

This change is to allow for differentiating .tfvars and .tf files when
using a terraform-ls language server.

The .tmLanguage syntax files are not changed in the same way as those
don't support including other syntaxes so would have to be duplicated
instead. And since those are long obsolete (in ST at least), I didn't
see much point in doing anything with them.

Fixes #49

The main syntax no longer handles the .tfvars extension. Handling of
.tfvars is now done by the new syntax file which also assigns a new
source.terraform-vars scope instead of the previous source.terraform.

This change is to allow for differentiating .tfvars and .tf files when
using a terraform-ls language server.

The .tmLanguage syntax files are not changed in the same way as those
don't support including other syntaxes so would have to be duplicated
instead. And since those are long obsolete (in ST at least), I didn't
see much point in doing anything with them.
@rchl
Copy link
Contributor Author

rchl commented Jun 16, 2021

The .tmLanguage syntax files are not changed in the same way as those
don't support including other syntaxes

Not 100% sure about that... If the including is possible then I could consider handling those too.

As for tests for tfvars file, I've just duplicated the existing one. That new syntax test file could be trimmed to only include tests for features that are specific to tfvars files but the syntax supports everything anyway so not sure.

Copy link
Collaborator

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

The .tmLanguage syntax files are not changed in the same way as those don't support including other syntaxes so would have to be duplicated instead.

That's sad but understandable, given the age of the format and its fading support in ST.

One more question:

Is there any convention for the *.sublime-settings and *.sublime-syntax files? I would expect these to follow Language ID, rather than file extensions? i.e. I'd expect terraform-vars.sublime-settings and terraform-vars.sublime-syntax, rather than tfvars.

Terraform-tfvars.sublime-syntax Outdated Show resolved Hide resolved
@radeksimko radeksimko self-requested a review June 18, 2021 10:19
Copy link
Collaborator

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I can confirm that it works nicely with the LSP package:

Screenshot 2021-06-21 at 11 11 24

@radeksimko
Copy link
Collaborator

radeksimko commented Jun 21, 2021

Worth noting that I had to update the client settings within the LSP plugin settings:

	"terraform":
		{
			"enabled": true,
			"languages": [
				{
					"languageId": "terraform",
					"document_selector": "source.terraform"
				},
				{
					"languageId": "terraform-vars",
					"document_selector": "source.terraform-vars"
				}
			],
			"syntaxes":
			[
				"Packages/Terraform/Terraform.sublime-syntax"
			],
			"command": [
				"terraform-ls",
				"serve"
			],
		}

@radeksimko radeksimko merged commit 329a58d into alexlouden:master Jun 21, 2021
@rchl rchl deleted the fix/separate-tfvars branch June 21, 2021 10:20
@rchl
Copy link
Contributor Author

rchl commented Jun 21, 2021

Created sublimelsp/LSP-terraform#1 for updating LSP-terraform.

(You've accidentally used typescript language ID in your config)

@rchl
Copy link
Contributor Author

rchl commented Jun 21, 2021

BTW. In ST4 it should work to remove the languages and syntaxes settings and use "selector": "source.terraform | source.terraform-vars" instead.

rchl added a commit to sublimelsp/LSP-terraform that referenced this pull request Jun 21, 2021
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.

None yet

2 participants