-
Notifications
You must be signed in to change notification settings - Fork 588
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
[JavaScript] Use meta.mapping instead of meta.object-literal #2020
Conversation
@@ -218,10 +218,10 @@ let x = import.meta; | |||
// method body, but we include it here to ensure that highlighting is not | |||
// broken as the user is typing | |||
let a = { otherIdentifier, foo(), baz: 1 } | |||
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ meta.object-literal | |||
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ meta.mapping |
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.
This wouldn't also satisfy meta.mapping - meta.mapping meta.mapping
, would it? Because I'm not seeing a clear_scopes
in this diff, especially not for object-literal-meta-key
.
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.
Good point. One issue is that there's an extra meta scope around the key and value when the value is a function:
+{
funcKey: function() {
// ^^^^^^^^^^^^^^^^^^^ meta.mapping meta.function.declaration
// ^^^^^^^ meta.mapping meta.function.declaration meta.mapping.key entity.name.function
},
}
Wrapping the whole thing like that in meta.function
is admittedly dubious, but it's used for the symbol list. Do you have any ideas on how to address this?
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.
@FichteFoll Any thoughts on this?
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.
I think this is reasonable for now. The pattern of naming functions via object key is so pervasive that we definitely wouldn't want to lose that functionality.
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.
Well, you could pop two scopes for the mapping key and re-push meta.mapping.key meta.function entity.name
(and do the same for the value), but unfortunately scope mangling like this can get extremly annoying when a set
is involved.
There is a solution to this, but having meta.mapping
is already an improvement over meta.object-literal
.
It looks like this has some assertion failures |
…hq#2020) * Use meta.mapping instead of meta.object-literal * Remove dollar scopes from meta.mapping.
meta.object-literal
→meta.mapping
meta.object-literal.key
→meta.mapping.key
Also added the missing
meta.mapping.key
to computed property names.