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

Improve attribute name matching #49

Merged
merged 3 commits into from
Dec 5, 2022
Merged

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Dec 2, 2022

This PR improves the attribute name matching in two ways:

  • We now match a whole word with the negative lookahead instead of just the first letter
  • We only match null, false, and true when they are followed by whitespace, so words like nullable are matched correctly.

UX

Before

CleanShot 2022-12-02 at 15 47 01@2x

After

CleanShot 2022-12-02 at 15 45 42@2x

Fixes hashicorp/vscode-terraform#1286

It will now correctly match words starting with null,
like nullable, but not match null as an attribute name.
Same for false and true.
@dbanck dbanck added the bug Something isn't working label Dec 2, 2022
@dbanck dbanck self-assigned this Dec 2, 2022
@dbanck dbanck requested a review from a team December 2, 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.

I'm guessing the reason we also still match null, true and false as some kind of "special" attribute names, although they are not really special, is pure pragmatism as it would be harder not to highlight, due to the regexp-based matching in TextMate, right?

With that in mind, LGTM. :shipit:

@radeksimko
Copy link
Member

radeksimko commented Dec 5, 2022

Oh, actually, one more note/suggestion - AFAICT technically this would be valid (although mis-formatted) config:

variable "foo" {
  nullable=true
  falseyy=""
  trueyy=""
}

Wouldn't that suffer from the same problem?

@dbanck
Copy link
Member Author

dbanck commented Dec 5, 2022

I'm guessing the reason we also still match null, true and false as some kind of "special" attribute names, although they are not really special, is pure pragmatism as it would be harder not to highlight, due to the regexp-based matching in TextMate, right?

We match null, true, and false as constant.language.hcl (Language Constants) here, although they aren't valid constants here. I think turning the constant matching off for attribute names would be hard.

Wouldn't that suffer from the same problem?

Good catch! I've updated the Regex and the test case for attributes without whitespace before the =.

@dbanck dbanck merged commit d6d7cd6 into main Dec 5, 2022
@dbanck dbanck deleted the b-improve-attribute-matching branch December 5, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nullable argument in variable block not properly highlighted
2 participants