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

Add new scope for block type and name #934

Merged
merged 1 commit into from
Feb 18, 2022
Merged

Conversation

jpogran
Copy link
Contributor

@jpogran jpogran commented Feb 10, 2022

This changes the existing block matcher to differentiate the type and name of a given block.

Previously the type and name of a block were scoped as string.quoted.double. This change scopes them to an entity.name.tag, which is commonly used to indicate a tag or attribute.

This will match Terraform blocks like resource "aws_instance" "web" { or module "foo" {

image

This will match Terraform blocks like module "foo" { or variable.

image

This will leave alone blocks without types or names:

image

Note: The LS needs to be disabled for you to see this until the semantic tokens are updated in the terraform-ls project.

Fixes #710

@jpogran jpogran self-assigned this Feb 10, 2022
@jpogran jpogran added the syntax label Feb 10, 2022
@jpogran jpogran force-pushed the resource_label_highlight branch 2 times, most recently from 76a66f4 to 71000dd Compare February 10, 2022 15:52
@jpogran jpogran changed the title resource label highlight Add new scope for block type and name Feb 10, 2022
@jpogran jpogran marked this pull request as ready for review February 10, 2022 16:35
@jpogran jpogran requested a review from a team February 10, 2022 16:35
Copy link
Member

@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.

FWIW Terraform/HCL does support unquoted labels too, e.g. resource aws_instance test { }. It's not considered "best practice" (at least you would not find it in any snippets of code on terraform.io) but it is supported and will likely remain supported in the foreseeable future as there are compatibility promises that were already made.

That's not to say the grammar has to account for that - I think it's a relative minority using that syntax, but given this fact I'm wondering if we should be reporting both enclosing quotes as part of the "label", effectively highlighting them the same colour as the characters within it?

@dbanck
Copy link
Member

dbanck commented Feb 10, 2022

Do you know if it's allowed to use string interpolation in labels? Do we need to account for this here?

@radeksimko
Copy link
Member

Do you know if it's allowed to use string interpolation in labels? Do we need to account for this here?

Not that I'm aware of - at least not the interpolation we think of in HCL traditionally - i.e. hcl.Expression.

Packer does use labels which seem to refer to other blocks of code (similar to standard references), but under the hood HCL AFAIK still treats this as string and I'm relatively sure they have to deal with that string internally within Packer somehow.
https://www.packer.io/docs/templates/hcl_templates/contextual-variables#source-variables

https://pkg.go.dev/github.com/hashicorp/hcl/v2#Block

type Block struct {
	Type   string
	Labels []string
// ...
}

@jpogran
Copy link
Contributor Author

jpogran commented Feb 11, 2022

Our existing regex doesn't account for blocks without quotes, the LS saves us by recognizing it:

image

I don't think we should expand scope of this by trying to address legacy behavior. We can ticket an investigation of whether we can have two behaviors inside one regex in another ticket.

I'm partial to the differently highlighted quotes, I think it helps identify different parts easier, but can see where it would conflict with the way interpolated strings look. Let's ask a few others for input and see what feedback we get

@jpogran jpogran requested a review from a team February 17, 2022 18:49
@jpogran
Copy link
Contributor Author

jpogran commented Feb 17, 2022

Team agreed to keep the string quotes and content the same color/token. PR updated and images adjusted

This changes the existing block matcher to differentiate the type and name of a given block.

Previously the type and name of a block were scoped as `string.quoted.double`. This change scopes them to an `entity.name.tag`, which is commonly used to indicate a tag or attribute.

This will match Terraform blocks like `resource "aws_instance" "web" {` or `module "foo" {`
Copy link
Member

@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.

LGTM 👍🏻

Non-blocking question:
I understand that not everything in TM may perfectly apply to every language but was there a particular reason to choosing entity.name.tag over entity.name.section or (TM undocumented) entity.name.label? They all seem to be somewhat applicable.

https://macromates.com/manual/en/language_grammars

  • tag — a tag name.
  • section — the name is the name of a section/heading.

@jpogran
Copy link
Contributor Author

jpogran commented Feb 18, 2022

Good question. I answered somewhat in PR description, but as you noted there isn't a direct 1:1 nomenclature here, and some of it is open to interpretation. The next challenge is picking something that themes will pick up on. Not all applicable scopes are assigned color values. It is mostly trial and error finding out which of the most popular/common themes choose to highlight. We could start providing themes, but that is out of scope of this ticket.

I did try out entity.name.label.terraform, entity.name.section and entity.other.attribute-name and a few others.

The entity.name.label or entity.name.section scopes sound like they are close to what we mean, but produce the same color as bare words, which makes it appear not highlighted in most themes:

image

The entity.other.attribute-name scope sounded close to what we mean by provider label, kinda, but highlights like a property, which visually is confusing right next to block attributes:

image

So I chose tag because it was the concept is closest to the idea of what we mean by provider while producing a visually different color in most of the themes I tested.

@jpogran jpogran added this to the 2.20.0 milestone Feb 18, 2022
@jpogran jpogran merged commit 0053fcf into main Feb 18, 2022
@jpogran jpogran deleted the resource_label_highlight branch February 18, 2022 17:52
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Color syntax: add scope for resource_type and resource_name
3 participants