Add more scopes to the tree-sitter grammar

atom / language-javascript

JavaScript language package for Atom

Other

194 stars 236 forks source link

Add more scopes to the tree-sitter grammar #620

Closed Ben3eeE closed 6 years ago

Ben3eeE commented 6 years ago

Fixes https://github.com/atom/language-javascript/issues/619 Fixes https://github.com/atom/language-javascript/issues/617 Fixes https://github.com/atom/language-javascript/issues/616 Fixes https://github.com/atom/language-javascript/issues/566

/cc: @maxbrunsfeld

Ben3eeE commented 6 years ago

@chbk This fixes the issues you reported earlier except default is staying scoped as keyword.control

chbk commented 6 years ago

Great! I tried out your PR and it fixes most issues except:

this is still source.js
super and super() are scoped differently

Ben3eeE commented 6 years ago

Yeah super() is a function call and is expected to be scoped differently, while super is a language variable and scoped as such,.

We scope: call_expression > super as support.function and super as variable.language

this is because I messed up when staging. Thanks for noticing and testing 👍 💯 Fixed in the latest commit.

chbk commented 6 years ago

Alright, super() being scoped differently makes sense. Would it be possible be more specific then, with support.function.super for example? It would be nice to be able to highlight super the same way in both cases, even if they are syntactically different. After all they refer to the same parent class, super() calling its constructor and super accessing any of its methods.

Ben3eeE commented 6 years ago

I'm ok with making it support.function.super. It still looks the same with one-dark-syntax and one-light-syntax.

@maxbrunsfeld I think this is ready for review now 😅

Ben3eeE commented 6 years ago

@chbk We removed some scope mappings for language.variable before merging this to get the other changes out in Atom 1.32.2. language.variable is awaiting review of https://github.com/atom/atom/pull/18383 before we change them.