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

Support semanticolor #20212

Merged
merged 8 commits into from Aug 11, 2020
Merged

Support semanticolor #20212

merged 8 commits into from Aug 11, 2020

Conversation

sharedprophet
Copy link
Contributor

@sharedprophet sharedprophet commented Dec 6, 2019

Issue or RFC Endorsed by Atom's Maintainers

#19151

Description of the Change

Pass node text to grammar for leaf parser nodes.

Alternate Designs

This is the only way for grammars to use the text to determine styling.

Possible Drawbacks

Performance penalty of unmarshalling from native text buffer is mitigated by only passing text for leaf nodes.

Verification Process

I have been running with this locally for over a week with no issues.

Release Notes

Pass syntax node text to grammar for leaf parser nodes, to support grammars that determine styling based on token values.

@sharedprophet
Copy link
Contributor Author

sharedprophet commented Apr 8, 2020

Re: performance penalty, I am referring to @maxbrunsfeld's comment here:

The .nodeText accessor on the TreeCursor has some performance cost. It needs to read a string from the native text buffer, and for internal syntax nodes (like a function declaration) it may be a very big string. And that line of code executes many times per syntax-highlighted line. So I'd like to avoid that performance cost if possible.

This solution avoids calling the .nodeText accessor on most long strings by only calling it on syntax nodes with no children.

@ocoka
Copy link

ocoka commented Apr 12, 2020

Hi to all Atom comunity, and all mantainers, if you can, you have a time and possibility, please promote this PR to positive decision to merge. Today more and more plugin developers are living for VSCode, but there are several gems that make Atom still the best, and great "SemanticColor" extension is one of them

@HugoMcPhee
Copy link

HugoMcPhee commented May 5, 2020

Is there anything needed to help to get this through?
For a temporary fix, semanticolor still works in separately downloadable atom v1.42
Update Edit:
Hi this change would make a huge difference for semanticolor users, and from what it looks like no performance issues, It would be great to get some feedback on this PR, thanks for the work on atom

@lkashef
Copy link
Contributor

lkashef commented Jun 16, 2020

@sharedprophet could you please make sure the CI is green, currently it's failing due to lint errors.

You can check the linting, by navigating to your repo and running script/lint from command line.

@sharedprophet
Copy link
Contributor Author

sharedprophet commented Jun 16, 2020

Looks like some unrelated tests timed out on Win64...?

@lkashef
Copy link
Contributor

lkashef commented Jun 23, 2020

@sharedprophet re-running the ci

@sharedprophet
Copy link
Contributor Author

sharedprophet commented Jun 24, 2020

Win64 tests seem flaky... something is up with the promises on there. It is failing on a PR to fix typos and that doesn't seem right.

@ocoka
Copy link

ocoka commented Jul 20, 2020

Hello, as I see PR is ready and reviewed, but CI failed for non related stuff... What is the next step, maybe rerun CI again, and cross the fingers ? )

@ocoka
Copy link

ocoka commented Jul 25, 2020

image
Hi, strange thing ... but oldier builds than that run is present

@lkashef
Copy link
Contributor

lkashef commented Aug 7, 2020

@ocoka
Copy link

ocoka commented Aug 8, 2020

Hello,
in the link, provided by @lkashef all state are green,
image
why CI state in this thread still shows some errors ?!

@lkashef
Copy link
Contributor

lkashef commented Aug 11, 2020

Thanks @ocoka for the heads up.

When the build is expired/deleted from Azure Devops, and a new manual run (instead of pushing a dummy commit) against the PR doesn't get picked up by the Github, that's why I provided the link above.


Thanks @sharedprophet for the contribution, I will go ahead and merge this PR

@lkashef lkashef merged commit 212081e into atom:master Aug 11, 2020
1 check failed
@jeff-hykin
Copy link

jeff-hykin commented Aug 11, 2020

Is there a place I can find more info on this? (What changed with regard to accessing text nodes inside grammars)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Aug 11, 2020

@jeff-hykin the diff is pretty small, and there is a new test that presumably demonstrates the functionality. I hope that can shed some light on the changes.

@sharedprophet
Copy link
Contributor Author

sharedprophet commented Aug 11, 2020

@jeff-hykin inside a grammar, the change will sometimes provide the node text as a second parameter to the idForScope function. You can only take advantage of this if you are creating a grammar with code rather than a normal TreeSitter or TextMate grammar file, though.

@jeff-hykin
Copy link

jeff-hykin commented Aug 11, 2020

@sharedprophet Ah thanks for the explanation that's what I was curious about 👍

And thanks @DeeDeeG

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

6 participants