-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(repl): Add color to functions #15434
Conversation
cli/tools/repl/editor.rs
Outdated
@@ -304,7 +305,9 @@ impl Highlighter for EditorHelper { | |||
fn highlight<'l>(&self, line: &'l str, _: usize) -> Cow<'l, str> { | |||
let mut out_line = String::from(line); | |||
|
|||
for item in deno_ast::lex(line, deno_ast::MediaType::TypeScript) { | |||
let mut lexed_items: VecDeque<_> = | |||
deno_ast::lex(line, deno_ast::MediaType::TypeScript).into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe using peakable()
might be more appropriate (https://doc.rust-lang.org/std/iter/struct.Peekable.html)? Then you can use lexed_items.next()
and lexed_items.peek()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if matches!(next, Some(deno_ast::TokenOrComment::Token(next_token)) if matches!(next_token, Token::LParen)) { | ||
// We're looking for something that looks like a function | ||
// We use a simple heuristic: 'ident' followed by 'LParen' | ||
colors::intense_blue(&line[range]).to_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. Is there any precedence where other people are doing this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I just used this trick before for a repl I have and it worked well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But its just a heuristic, so it only works for the most part, which I think its fine personally for a repl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should apply same coloring as in diagnostics coming from TSC or the linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, by precedence I just meant someone else using a different colour for the function names. The heuristic looks good. Anyway, not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pr adds colors to functions
This is all subjective but I thought its nicer to have more colors in general.