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

Split the code doing highlighting from the rest. #37

Open
wants to merge 1 commit into
base: master
from

Conversation

@nickolay
Copy link

@nickolay nickolay commented Mar 8, 2016

Currently highlights.coffee mixes distinct functionality:

  • finding, loading, and selecting grammars to use via first-mate module. This has many dependencies and is a large part of the whole module.
  • returning the HTML for highlighting, given tokens from first-mate's grammar. It is self-contained and pretty straightforward, when you know what it does.

I suggest splitting the two for the sake of those assessing the library.

My story: I was looking for a library to perform syntax highlighting based on AST. I did not find it yet, but found this library.
I got confused by the "Loading Grammars From Modules" section of the README, as it seemed to suggest there was a way to write a JS module to perform the syntax highlighting, but provided no information on the API of such module, so I went looking into the source code to find what kind of input the highlighting code expects from the tokenizer.

(Turns out you only support TextMate-style language definitions, which are handled by first-mate, but the bit of code I'm splitting into a separate file would still be reusable in my case.)

@bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 12, 2016

@nickolay mind rebasing with master, also I'd love to know @kevinsawicki's thoughts on this refactor.

@nickolay
Copy link
Author

@nickolay nickolay commented Aug 12, 2016

@bcoe Thanks for getting back to me! Sure, if you decide this split-up is something you want, I can prepare an updated PR.

@bcoe
Copy link
Contributor

@bcoe bcoe commented Jul 8, 2017

@nickolay if you're still interested in landing this refactor, please go ahead and update this pull request and I'll work on getting it landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants