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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom modifiers via BlockSchema & LabelSchema #106

Merged
merged 5 commits into from
Mar 16, 2022

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Mar 15, 2022

The LSP spec is not explicit in saying that custom token types or modifiers are supported, but it does neither explicitly say it's forbidden.

The fact that the underlying type for both is a string and that there is relatively detailed corresponding client/server capability available suggests this is possible.

The expectation is that downstream LS (terraform-ls) would use the client capability to see what token modifiers the client actually supports before offering them any custom ones and it would continue providing fallback, just like terraform-ls does today:

https://github.com/hashicorp/terraform-ls/blob/2de83c81f9e53457b943efaf0801acf6185b71b3/internal/lsp/token_encoder.go#L37-L86

The benefit is that clients which do support more specific token types and modifiers can use those - which in turn enables more specific themes that can take advantage of these. Clients which do not support these will continue to use the fallback types & modifiers. So basically - everyone's happy? 馃槃 馃馃徎

Related: hashicorp/vscode-terraform#958

This enables downstream schemas to declare their own modifiers, e.g. "terraform-resource".
@radeksimko radeksimko added the enhancement New feature or request label Mar 15, 2022
@radeksimko radeksimko force-pushed the f-custom-token-modifiers branch 2 times, most recently from 92d6e21 to 35625f5 Compare March 15, 2022 19:02
@radeksimko radeksimko marked this pull request as ready for review March 15, 2022 19:46
@radeksimko radeksimko self-assigned this Mar 15, 2022
@radeksimko radeksimko requested a review from a team March 15, 2022 19:52
This makes it possible to use the types directly downstream without having to convert them into strings.
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Nice addition 馃憤

Do you think it would be useful to have semantic token modifiers on attributes, too?

@radeksimko
Copy link
Member Author

Do you think it would be useful to have semantic token modifiers on attributes, too?

Potentially yes, although I imagine traversals would be wanted more. I wanted to ship at least basic support for this though. We can always add more later, removing is typically harder once people rely on it and I'm not entirely sure what the best modifiers would be in the context of traversals and attributes yet.

We already set dependent modifier for some attributes, like module source or resource provider, which folks may want to highlight differently. I'm guessing some might want all core/meta-attributes to be highlighted differently? but I don't know that for sure.

Similarly for traversals folks may want to highlight the whole address (e.g. data.aws_instance.name) depending on what logical block it belongs to - but I don't know for sure. Some folks wanted to highlight the first "root" segment - e.g. data in data.aws_instance.name but that leaves question marks for resource addresses which do not have any such dedicated segment.

So ... TL;DR lots of questions to answer still IMO.

@radeksimko radeksimko merged commit 49ffde6 into main Mar 16, 2022
@radeksimko radeksimko deleted the f-custom-token-modifiers branch March 16, 2022 20:48
@dbanck
Copy link
Member

dbanck commented Mar 17, 2022

Thanks for the explanation and for raising more questions 馃槃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants