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 HCL syntax into its own file #56

Merged
merged 4 commits into from
Jan 18, 2024
Merged

Conversation

rchl
Copy link
Member

@rchl rchl commented Jan 17, 2024

Per discussions in sublimelsp/LSP-terraform#26 and in #48 I've extracted support for .hcl files into a dedicated syntax that inherits the main syntax.

The point of it is to make .hcl extension not be handled by the main syntax so that it doesn't get the same source.terraform scope but a dedicated source.terraform-hcl source.hcl scope. This is required for Sublime LSP as it enables language server based on the scope and it isn't possible to exclude .hcl from starting the server otherwise.

tm-language syntax is unaffected.

Resolves sublimelsp/LSP-terraform#26
Resolves #48

@radeksimko

Terraform-hcl.sublime-syntax Outdated Show resolved Hide resolved
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.

In general we tend to treat HCL-based languages and HCL itself as separate languages entirely on the surface and keep the relationships/inheritance more as an implementation detail. I know in this case the HCL grammar is not strictly a generic enough HCL grammar today (it remains Terraform-specific) but I'd assume that is the hope for the future?

I guess a more radical (cleaner?) solution here would be to even have a separate repository HCL.tmLanguage where HCL is the base and Terraform grammar is co-hosted there or remains in Terraform.tmLanguage.

In other words, users of HCL-based languages who don't use Terraform should not need to know about Terraform, see anything Terraform related or install anything with Terraform in the name.

All that said I feel I would be asking for much more of your valuable contributor time than you may be willing to invest into this and so I will keep my comments/suggestions focused merely on the scope.

TL;DR Is there a reason we should be including both HCL and Terraform in the scope?

@rchl
Copy link
Member Author

rchl commented Jan 17, 2024

I guess a more radical (cleaner?) solution here would be to even have a separate repository HCL.tmLanguage where HCL is the base and Terraform grammar is co-hosted there or remains in Terraform.tmLanguage.

Maybe I don't fully understand your comments and also I certainly don't fully understand how tmLanguage syntaxes work but I don't think tmLanguage-based syntaxes can extend each other (I could be wrong). If that's the case then does that make your idea hard or impossible to do? I guess the inheritance could still be done in the form of built-time syntax generation and result in duplicated code in each syntax but I'm not sure how would that be implemented and it sounds like too much effort for me.

TL;DR Is there a reason we should be including both HCL and Terraform in the scope?

You are talking about tmLanguage but I'm only handling sublime-syntax variants here. So in that case would you suggest creating HCL.sublime-syntax and naming it just HCL? I could do that but I think it would still need to be part of this repo and included in the same ST package as otherwise extending would not work unless both packages are installed.

@radeksimko
Copy link
Collaborator

radeksimko commented Jan 18, 2024

I don't think tmLanguage-based syntaxes can extend each other (I could be wrong). If that's the case then does that make your idea hard or impossible to do?

There are some ways to inherit between TM grammars "natively" but they're not very intuitive nor flexible and we abandoned attempts of using it very early on. I don't know enough about Sublime syntax grammars to suggest if there is anything comparable.

I guess the inheritance could still be done in the form of built-time syntax generation and result in duplicated code in each syntax but I'm not sure how would that be implemented and it sounds like too much effort for me.

That is exactly what we do for our TM grammars and totally understandable that would be quite a lot of effort.

I was merely suggesting that regardless of the exact grammar rules being Terraform specific, we can report *.hcl files as scope.hcl and deal with that tech debt (i.e. lack of truly generic HCL grammar) at some point in the future. If the solution happens to be an entirely separate repository/package then I assume that we can remove *.hcl from this grammar and let it be focused on Terraform (*.tf, *.tfvars) only. This is all "to be solved in the future" though, for now I think it's acceptable that we use Terraform grammar to highlight HCL files. What matters more is that we report those HCL files as HCL.

You are talking about tmLanguage but I'm only handling sublime-syntax variants here.

I am talking about both actually since the PR is modifying Comment.tmPreferences (tm == TextMate AFAIK) and *.sublime-syntax. Right now, AFAICT the proposed change would result in *.hcl files being reported with source.terraform-hcl scope. That is certainly better than source.terraform but I think it should be just source.hcl for all the reasons mentioned above and also in the code comment:

# - ".hcl": non-terraform tools often use this HCL syntax, i.e. Vault
#   https://www.vaultproject.io/docs/configuration/

i.e. Vault configuration file should not be reported as scope that has "terraform" in the name, I think, regardless of how exactly the highlighting grammar works.


I hope that answers your questions, I will try to stay available on the Sublime Text Discord in the coming days in case you want to clarify/discuss this further in a more synchronous way.

@radeksimko
Copy link
Collaborator

One more point of clarification:

I would be open to *.tf files being reported as e.g. source.hcl.terraform and *.tfvars as source.hcl.terraform-vars but I think that is unrelated to the problem you are trying to address here, which is treatment of (non-Terraform) HCL files.

@rchl
Copy link
Member Author

rchl commented Jan 18, 2024

I've renamed the file to HCL.sublime-syntax and updated the scope to scope.hcl.

I agree that source.hcl.terraform scope for the main syntax makes sense but it would be disruptive change for LSP and possibly not only for it.

@rchl
Copy link
Member Author

rchl commented Jan 18, 2024

As far as CI being stuck, maybe https://github.com/alexlouden/Terraform.tmLanguage/blob/7faec10b4057b95bff82fb585730e560871d26c4/.github/workflows/ci.yaml#L15 should be changed to ubuntu-latest. Maybe runners for that specific version are not available or something...

@radeksimko
Copy link
Collaborator

@rchl you were right about the CI, I addressed that in #57 - can you try rebasing your PR now?

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.

This now LGTM, happy to merge once CI is green (hopefully just a matter of rebasing)

* upstream/master:
  github: bump linux version to latest
@radeksimko
Copy link
Collaborator

Thanks for the PR, I will look into cutting a release hopefully today (with this change).

@radeksimko radeksimko merged commit 7ebc850 into SublimeText:master Jan 18, 2024
2 checks passed
@rchl rchl deleted the feat/hcl branch January 18, 2024 11:46
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.

Syntax matching Separate syntax for .tfvars
2 participants