atom / language-javascript

JavaScript language package for Atom
Other
194 stars 236 forks source link

Colorize arrow function parameters without parens #652

Closed caleb531 closed 5 years ago

caleb531 commented 5 years ago

Requirements

Description of the Change

This PR tokenizes single (paren-less) parameters for arrow functions. The first-mate source.js grammar already did this, while the current tree-sitter grammar does not. Consider the following screenshots:

Current tree-sitter grammar:

1-before

Tree-sitter grammar with this PR applied:

2-after

Current first-mate grammar:

3-firstmate

Alternate Designs

A different selector could be used or the precedence of the grammar rule could be adjusted, if need be. Although this seems to work without any repercussions.

Benefits

More consistent coloring for arrow function parameters, whether or not you use parens.

Possible Drawbacks

I suppose if the selector chosen tokenizes more than it should, but I tried to keep it simple enough that it shouldn't be a concern.

Applicable Issues

N/A

rsese commented 5 years ago

Thanks again @caleb531, someone will take a look as soon as they can :v:

rsese commented 5 years ago

Failure looks unrelated, we restarted it but same error.

jasonrudolph commented 5 years ago

:wave: Hi @caleb531: Thanks for this! :bow: I tested this out locally, and I do see that this pull request causes the arrow function parameters to have a new scope, but I don't see any change to the syntax highlighting. It's possible that I'm doing something wrong, so I'll show you what I'm seeing, and you can let me know if I'm missing something. 😅

Note: All of the examples below are using Atom 1.42.0-nightly1 and the monokai syntax theme.

With Tree-Sitter parsers disabled

Screen Shot 2019-08-14 at 5 35 26 PM

With Tree-Sitter parsers enabled

Screen Shot 2019-08-14 at 5 36 46 PM

With Tree-Sitter parsers enabled using the changes from this pull request

Screen Shot 2019-08-14 at 5 36 16 PM

I see that the formal-parameter scope gets applied, but I'm not sure if syntax themes are using that scope or not. Instead of applying the formal-parameter scope, should we instead apply the same scopes used in the TextMate grammar (i.e., meta-method-call meta.arguments meta.function.arrow meta.parameters variable.parameter.function)?

caleb531 commented 5 years ago

@jasonrudolph Ha, that's because it turns out I have the following styles in my styles.less:

atom-text-editor:not([mini]) .syntax--source.syntax--js .syntax--formal-parameter.syntax--identifier {
   color: @orange;
}

My apologies about that. But I agree, variable.parameter.function would be the way to go (as this scope is already used in the Python tree-sitter grammar, and the color of those parameters is already orange for monokai, as it should be)

jasonrudolph commented 5 years ago

But I agree, variable.parameter.function would be the way to go...

@caleb531: Cool. Wanna make that change?

caleb531 commented 5 years ago

@jasonrudolph Just pushed the rebased changes! The scope is now variable.parameter.function.

jasonrudolph commented 5 years ago

Just pushed the rebased changes!

Thanks, @caleb531! 🚀