-
Notifications
You must be signed in to change notification settings - Fork 129
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
Something i forgot to mention: since |
@@ -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 |
There was a problem hiding this comment.
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
9c94065
to
4800753
Compare
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 |
@@ -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 |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
38ce771
to
0e1f58e
Compare
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!).