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 scopes of type cast declarations #798

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

forivall
Copy link

@forivall forivall commented Jan 2, 2020

I'm working on some syntax highlighting to better disambiguate what is type annotations and what is actual executable code, and I found that scope information in type casts were missing.

Let me know if there's better scope namings for any of these, I based it on what names made sense based on tm scope standards & what's elsewhere in this tmLanguage definition. Or, if this change can't be made, let me know why (no worries!).

@@ -67,37 +67,37 @@ Grammar: TypeScript.tmLanguage
^
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.brace.angle.ts
^
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.type.function.ts meta.parameters.ts punctuation.definition.parameters.begin.ts
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.type.cast.ts meta.type.function.ts meta.parameters.ts punctuation.definition.parameters.begin.ts
Copy link
Member

Choose a reason for hiding this comment

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

As you can see there is alreadycast.expr.ts so this change isn't needed.

Copy link
Author

@forivall forivall Jan 3, 2020

Choose a reason for hiding this comment

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

cast.expr.ts includes the surrounding < and >, and i have meta.type.cast.ts include only the contents. (see diffs at https://github.com/microsoft/TypeScript-TmLanguage/pull/798/files#diff-d5222b162266b45fd998419db416fdddR1577 and https://github.com/microsoft/TypeScript-TmLanguage/pull/798/files#diff-d5222b162266b45fd998419db416fdddR1591

In my use case, i want to change the syntax highlighting of the contents of the cast, but not the surrounding punctuation (< >) or the as.

That was something i forgot to mention: since cast.expr.ts is used for <MyType>variable style-casts, should variable as MyType style casts also use cast.expr.ts, or is meta.type.cast.ts preferable? If they are switched to cast.expr.ts, should the as also have cast.expr.ts?

Also, for the sake of consistency, it made sense to also apply meta.type.cast.ts to these, even though they already have cast.expr.ts.

Copy link
Member

Choose a reason for hiding this comment

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

The scenario you are looking for can be achieved by omitting cast.expr.ts meta.brace.angle.ts. We want to keep this simple and not add unnecessary scopes. Most themes do not care about the difference and hence we wont be accepting this change

@forivall
Copy link
Author

forivall commented Jan 3, 2020

Something i forgot to mention: since cast.expr.ts is used for <MyType>variable style-casts, should variable as MyType style casts also use cast.expr.ts, or is meta.type.cast.ts preferable? If they are switched to cast.expr.ts, should the as also have cast.expr.ts?

@@ -67,37 +67,37 @@ Grammar: TypeScript.tmLanguage
^
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.brace.angle.ts
^
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.type.function.ts meta.parameters.ts punctuation.definition.parameters.begin.ts
source.ts meta.var.expr.ts meta.objectliteral.ts meta.object.member.ts cast.expr.ts meta.type.cast.ts meta.type.function.ts meta.parameters.ts punctuation.definition.parameters.begin.ts
Copy link
Member

Choose a reason for hiding this comment

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

The scenario you are looking for can be achieved by omitting cast.expr.ts meta.brace.angle.ts. We want to keep this simple and not add unnecessary scopes. Most themes do not care about the difference and hence we wont be accepting this change

@forivall
Copy link
Author

forivall commented Jan 4, 2020

I removed it that part. Personally, i'm not currently completely satisfied, since scope exclusion is limited with the currently available scope selectors so verbose workarounds are required (for example: microsoft/vscode-textmate#52 (comment) ). But hopefully that'll eventually be fixed, and my main focus is still to get a scope for the as casts.

@@ -36,9 +36,9 @@ Grammar: TypeScript.tmLanguage
^
source.ts meta.var.expr.ts
^
source.ts meta.var.expr.ts entity.name.type.ts
source.ts meta.var.expr.ts meta.type.cast.ts entity.name.type.ts
Copy link
Member

Choose a reason for hiding this comment

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

Please use cast.expr.ts as scope instead of meta.type.cast.ts

@@ -1375,7 +1375,7 @@ repository:
match: '{{startOfIdentifier}}(as)\s+(const)(?=\s*([,}]|$))'
captures:
'1': { name: keyword.control.as.ts }
'2': { name: storage.modifier.ts }
'2': { name: storage.modifier.const.type.ts }
Copy link
Member

Choose a reason for hiding this comment

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

please do not change scope for modifier const. Instead add name as cast.expr.ts

@@ -1561,7 +1561,7 @@ repository:
match: \s*(<)\s*(const)\s*(>)
captures:
'1': { name: meta.brace.angle.ts }
'2': { name: storage.modifier.ts }
'2': { name: storage.modifier.const.type.ts }
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -1624,11 +1624,12 @@ repository:
- match: '{{startOfIdentifier}}(as)\s+(const)(?=\s*($|[;,:})\]]))'
captures:
'1': { name: keyword.control.as.ts }
'2': { name: storage.modifier.ts }
'2': { name: storage.modifier.const.type.ts }
Copy link
Member

Choose a reason for hiding this comment

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

Same here

- begin: '{{startOfIdentifier}}(as)\s+'
beginCaptures:
'1': { name: keyword.control.as.ts }
end: (?=$|^|[;,:})\]]|\|\||\&\&|({{startOfIdentifier}}(as)\s+)|(\s+\<))
contentName: meta.type.cast.ts
Copy link
Member

Choose a reason for hiding this comment

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

Name this as cast.expr.ts and use name instead of contentName since cast expression starts at as

@@ -1375,7 +1375,7 @@ repository:
match: '{{startOfIdentifier}}(as)\s+(const)(?=\s*([,}]|$))'
captures:
'1': { name: keyword.control.as.ts }
'2': { name: storage.modifier.ts }
'2': { name: storage.modifier.const.type.ts }
- name: meta.object.member.ts
begin: '{{startOfIdentifier}}(as)\s+'
beginCaptures:
Copy link
Member

Choose a reason for hiding this comment

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

Add name for this as cast.expr.ts as well

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.

None yet

2 participants