hashicorp / vscode-terraform

HashiCorp Terraform VSCode extension
https://marketplace.visualstudio.com/items?itemName=HashiCorp.terraform
Mozilla Public License 2.0
911 stars 180 forks source link

v 1.3.12 breaks syntax colours #184

Closed fluffy-cakes closed 4 years ago

fluffy-cakes commented 5 years ago

module, vars, data, resources, all changed to one flat colour after update

Before image

After image

mauve commented 5 years ago

@MattFenner can you please have a look if your changes triggered this break?

MattFenner commented 5 years ago

This is likely due to my changes. I only fixed support for some of the basic cases, I knew there would be more work needed. But, I guess it broke some other cases I didn't test.

I don't have much time to look at the moment, so someone else is welcome to have a look if they want, otherwise maybe I will get a chance later in the week, but not promising anything.

MattFenner commented 5 years ago

So I had a look at this, and I am not convinced it is a bug. It does look different than before, but I think it is actually more correct than before.

here is what it looks like for me (abbreviated): image

here is what I think is a similar construct in javascript (for comparison): image

The colours look mostly the same.

also using the "inspect TM scopes" tool in VS Code you can see what it has assigned each of the parts of the code: image

image

what it is saying is that the left hand side is a variable assignment, and the variables inside the string interpolation are variables also, so they both get the same colour. Typescript has pretty much the same assigned token types.

Before my change the tokens were getting assigned the wrong types and were actually the wrong colour, but people were probably used to it.

I would love to hear your feedback @mauve and @fluffy-cakes, cause I am new to this, but it makes sense to me.

MattFenner commented 5 years ago

After writing all that, I realized am having trouble seeing anything different between the 2 versions for that particular snippet. Even when I go back to an older version i see the same colours (although it is different for the other things that I fixed). Not sure if i'm doing something wrong, or my machine is caching it or something.

@fluffy-cakes would you mind running the inspect TM scopes tool in VS code and show me what you get for the different bits of text, with the old and new versions.

thanks

fluffy-cakes commented 5 years ago

@MattFenner , I'll have to update to the new version again and do some tests. Probably won't be today, unfortunately, probably tomorrow.

fluffy-cakes commented 5 years ago

1.3.11 image

1.3.12 image

MattFenner commented 5 years ago

Thanks @fluffy-cakes, that's what I would have expected.

It is a bit subjective, but I would say that "variable.other.terraform" is more appropriate, since in my mind it is a variable.

But, I guess you could argue either way.

@mauve thoughts?

edit: I should mention that I copied these mappings from the official HCL2 repo, so it matches what they were thinking as well.

fluffy-cakes commented 5 years ago

The problem is that all of them seemed to have the same formatting attached. image

I use this add-in to help easily identify the difference between resources/modules/var/etc... and that the colour differs from the key/value. I know it sounds silly, but it makes reading TF a lot easier, which I thought was the idea behind this add-in.

MattFenner commented 5 years ago

I would love to hear some more opinions, i.e. from @mauve .

The keys and values are different colours, for string values etc. The places where they are the same colour is where you are referencing another variable, because in that scenario you are pointing one variable to another variable, so they are the same thing (in my mind) - both variables.

I don't think it was ever doing what you think it was doing. We could never make resources, modules, vars etc. different colours, like you say. The colourizer doesn't have that level of information. All it was doing before was that values within a string interpolation are a different colour to when they are defined (probably on accident).

This matches what other languages do as well (see my typescript example above), Here is another example from c# (note the second line where firstVariable and secondVariable are the same colours.

image

fluffy-cakes commented 5 years ago

OK, I now know what you mean by referencing another variable, hence the same colour, as opposed to being a string value.

image

I wonder how, before, that it was producing a different colour before; where reference values where one colour (in my instance, orange), and string values where another (in my instance, green). This would ideal, at least for me because... you know, pretty colours and whatnot, easier to identify parts of the code at a glance.

claydanford commented 5 years ago

Hi, just chimming in to say, it was better when the vars were a different color than the arguments on the left.

paultyng commented 4 years ago

We just released v2.0.0-rc.1 of the extension. The main features include:

You can find additional information and specifics in the release notes and CHANGELOG.

With this release we expect that many of the prior issues and PRs are no longer relevant or have been addressed, and are therefore being closed. If you feel the action taken on an issue or PR is in error, please comment as such and we can figure out the appropriate way to address it.

We plan to add the final 2.0.0 release to the marketplace soon, but are actively seeking your feedback now on the release candidates. You can download the .vsix from the releases page and manually install it in VS Code to try it out.

ghost commented 4 years ago

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.