feat: add syntax highlighting in the REPL #2254
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
kgryte
added
Enhancement
There was a problem hiding this comment.
Overall, this looks pretty good. I left a suggestion on how we could move the colors to their own package for further downstream use.
In general, I would suggest keeping the scope of this PR limited and do theming and settings in follow-up PRs. Tests and the non-TTY handling may be good as part of this PR.
My main other suggestion would be to see whether we want to support any other token types; it's probably better to be exhaustive here as this would then allow for more styling options. As a reference, here are the token types used by the popular highlight.js library:
https://highlightjs.readthedocs.io/en/latest/css-classes-reference.html
I propose going through their list and see whether there are additional token types to add to the tokenizer.js handling.
| */ | ||
| var ANSI = { | ||
| // Original colors: | ||
| 'black': '\u001b[30m', |
There was a problem hiding this comment.
Let's get this in for now as-is, but as a follow-up it may be worthwhile to add a separate package specifically for colors, as an alternative to https://www.npmjs.com/package/colors or https://www.npmjs.com/package/chalk. Similar to what chalk is doing, we could do feature detection in there for which ANSI colors are supported and adjust the exports accordingly. A potential location for such a package may be @stdlib/cli/colors, with color support detection potentially living in the assert namespace.
There was a problem hiding this comment.
That would be nice to have! will add it as a TODO.
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
@Planeshifter I went ahead and added more token types.
Punctuation and operators can also be colored now, I have decided not to with the default theme. I am planning to add tests together with settings and API in the follow-up PR as custom themes would make testing each token-type easier. Is that okay? I also tried adding escape sequences as a token type. I wrote a regex that matches any single character after |
|
@Planeshifter @kgryte I was also wondering if we could highlight output values (like |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
@Snehil-Shah I'd probably avoid highlighting return values, as string representations of return values may not be syntactically correct (e.g., Re: tests. Yes, deferring until we've added custom theming makes sense. As I believe you are suggesting, custom theming will allow toggling on and off token types, etc, thus allowing for easier unit tests. |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Snehil-Shah
force-pushed
the
syntax-highlighting
branch
from
6314daf
to
33eb6a0
Compare
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
@kgryte I have also decided to highlight when the user is declaring an existing variable as I feel it can help the user avoid writing conflicting variable names. So if the user goes |
@Snehil-Shah Yes, that seems reasonable. Some overlap with preview completions, which also hint at existing identifiers, but likely worth distinguishing. I suspect that most themes will make existing and new identifiers color the same, but there may be some which would like to distinguish as you propose. |
There was a problem hiding this comment.
@Snehil-Shah Is the PR ready for a final review? Would be great if we could get it in shortly and save follow-up work for subsequent PRs.
|
@Planeshifter Yes, it's ready! |
| var isStringTokenType = contains.factory( [ 'string', 'template' ] ); | ||
| var isLiteralType = contains.factory( [ 'string', 'boolean', 'number' ] ); | ||
| var isReservedName = contains.factory( [ 'undefined', 'async', 'await', 'let' ] ); | ||
| var RE_PUNCTUATION = /(\{|\}|\[|\]|\(|\)|,|;|:|\.|\?|\?\.|=>|\.\.\.|`|\${)/; |
There was a problem hiding this comment.
We could consider moving this into its own @stdlib/regexp/* package down the line.
| // VARIABLES // | ||
|
|
||
| var COMMANDS = commands(); | ||
| var isControlKeyword = contains.factory( [ 'if', 'else', 'switch', 'case', 'catch', 'finally', 'try', 'return', 'break', 'continue' ] ); |
There was a problem hiding this comment.
It may be a good idea to move this array and the ones below into a separate JSON file such as the one below:
{
"controlKeywords": ["if", "else", "switch", "case", "catch", "finally", "try", "return", "break", "continue"],
"specialIdentifiers": ["this", "super"],
"reservedLiterals": ["null", "true", "false"],
"unrecognizedKeywords": ["async", "await", "let"],
"stringTokenTypes": ["string", "template"],
"literalTypes": ["string", "boolean", "number"],
"reservedNames": ["undefined", "async", "await", "let"]
}Such separation of data from the code logic may be helpful when writing tests and would make reuse of these sets easier.
There was a problem hiding this comment.
I don't think these sets mean much apart from this code logic. For example, the keywords only contains the keywords that acorn doesn't recognise as a keyword token. So it kind of becomes specific to the tokenizer. We can still move it to a separate JSON if that makes the code cleaner. Should I make a separate module that returns the object or should I make a JSON file (and at which directory)?
There was a problem hiding this comment.
@Snehil-Shah That's fair; let's get the PR in now and we could still move it to its own file later if needed for testing etc. Since specific to the tokenizer, separate file would live in the same directory as the tokenizer.js file. In general, we prefer JSON when there is no reason that the exported object would ever be mutated, but use a module with a function instantiating the object when we want to allow returning a new copy.
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
PR-URL: stdlib-js#2254 Resolves: stdlib-js#2072 --------- Signed-off-by: Snehil Shah <snehilshah.989@gmail.com> Reviewed-by: Athan Reines <kgryte@gmail.com> Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>
PR-URL: stdlib-js#2291 Ref: stdlib-js#2254 Co-authored-by: Athan Reines <kgryte@gmail.com> Reviewed-by: Athan Reines <kgryte@gmail.com> Signed-off-by: Snehil Shah <snehilshah.989@gmail.com> Signed-off-by: Athan Reines <kgryte@gmail.com>
Resolves #2072
Description
This pull request:
settingsto control behavior and change themes.This PR is in progress. Requesting for feedback..
Related Issues
This pull request:
Questions
As there are only a few colors we can choose from to highlight using ANSI codes, I have kept a minimalist approach to highlighting. We don't highlight identifiers, operators and brackets. This is consistent with how IPython and node's repl-rewrite do their syntax highlighting. This can easily be changed as acorn's tokenizer is pretty verbose, so would love to hear your thoughts..
Other
Currently, I have only added one theme. I will be extending this and also going to add an API for user-defined themes.
Tokens gettings highlighted:
Attaching some screenshots at different terminal backgrounds:
Light:
Dark:
Monokai:
Solarized:
Checklist
@stdlib-js/reviewers