Skip to content

repl: support syntax highlighting - #53571

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:repl-syntax-highlighting
Closed

repl: support syntax highlighting#53571
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:repl-syntax-highlighting

Conversation

@avivkeller

@avivkelleravivkeller commented Jun 24, 2024

Copy link
Copy Markdown
Member

Instead of sending many REPL changes in one PR (#52965), it was recommended to me to implement these one-by-one in several PRs.

First up: Syntax highlighting 🎉

This PR adds RegEx-based syntax highlighting to the REPL.


Notable change text (if text this short is even worth it): "The REPL now includes syntax highlighting for input."

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Jun 24, 2024
@avivkelleravivkeller changed the title repl: enable syntax highlightingrepl: support syntax highlightingJun 24, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

CC @nodejs/repl

@avivkelleravivkeller mentioned this pull request Jun 24, 2024
3 tasks
@ZYSzys

Copy link
Copy Markdown
Member

Could you add some tests ?

@avivkeller

avivkeller commented Jul 2, 2024

Copy link
Copy Markdown
MemberAuthor

Tests have been added @ZYSzys, sorry I forgot to let you know when I pushed the commits.

The failed tests appear unrelated.

@avivkelleravivkeller added the review wanted PRs that need reviews. label Jul 9, 2024
Comment threadtest/parallel/test-repl-syntax-highlighting.js Outdated
Comment threadlib/repl.js Outdated
Comment threadlib/internal/repl/highlight.js Outdated
@avivkelleravivkeller removed the review wanted PRs that need reviews. label Jul 10, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

(got review - removing label)

@avivkeller

Copy link
Copy Markdown
MemberAuthor

@atlowChemi

All review conversations have been resolved, any more notes?

@BridgeAR

Copy link
Copy Markdown
Member

@redyetidev do you have some screenshots or a short video of the usage? I am not certain in what way the syntax highlighting has changed :)

@avivkeller

avivkeller commented Jul 11, 2024

Copy link
Copy Markdown
MemberAuthor

@BridgeAR highlighting is done to input, and it follows (mostly) the same rules as util.inspect.

New

image

Old

image


I'm not sure if this is semver-minor / notable-change

@MoLow

Copy link
Copy Markdown
Member

I'm not sure if this is semver-minor / notable-change

I think both :)

@avivkelleravivkeller added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Jul 14, 2024
@github-actions

Copy link
Copy Markdown
Contributor

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @redyetidev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@avivkeller

avivkeller commented Jul 14, 2024

Copy link
Copy Markdown
MemberAuthor

@MoLow ive added both labels.

@avivkeller

Copy link
Copy Markdown
MemberAuthor

All checks have passed, but I assume this needs a CI to land. If someone could review, approve, and CI, it would be appreciated. Thanks :-)!

@atlowChemiatlowChemi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR currently only tests the new internal/repl/highlight module, but I think tests should be aded to ensure the input is colored in REPL as well.

@avivkeller

avivkeller commented Jul 16, 2024

Copy link
Copy Markdown
MemberAuthor

The PR currently only tests the new internal/repl/highlight module, but I think tests should be aded to ensure the input is colored in REPL as well.

The highlight function is called directly within the REPL writing, so I'm not sure how to hook into it's input writing, but I believe the tests currently in use suffice.

@atlowChemi

Copy link
Copy Markdown
Member

so I'm not sure how to hook into it's output

I'd look at test like test-repl-autocomplete.js, test-repl-context.js, and test-repl-editor.js etc

@atlowChemiatlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@atlowChemiatlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 7, 2024
@avivkelleravivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 7, 2024
@avivkeller

avivkeller commented Aug 7, 2024

Copy link
Copy Markdown
MemberAuthor

I'm a little concerned about the failure at https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_withoutssl_x64/45088/testReport/junit/(root)/parallel/test_repl_preview_without_inspector/, but it didn't seem to fail anywhere else 🤔

I've removed author ready, because I don't think this should land until that is resolved. Feel free to re add the label if you disagree.

@avivkelleravivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 7, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Fixed the issue, readded the label.

@avivkelleravivkeller removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 8, 2024
@jasnelljasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024
@nodejs-github-bot

This comment was marked as outdated.

@avivkeller

Copy link
Copy Markdown
MemberAuthor

The failed CI is unrelated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@avivkeller

Copy link
Copy Markdown
MemberAuthor

https://ci.nodejs.org/job/node-test-commit-linux-containered/45732/ is failing, and that concerns me that this API is adding to many control sequences, and it'll clutter developers trying to simply parse the output. I'm wondering if this should be closed for now, until a time where it can be implemented with less control sequencnes?

input: " { b: 1 }['b'] === 1",
preview: [
" { b: 1 }['b'] === 1\r",
"\x1B[1G\x1B[0Jrepl > \x1B[9G\x1B[1G\x1B[0Jrepl > {\x1B[10G\x1B[1G\x1B[0Jrepl > { \x1B[11G\x1B[1G\x1B[0Jrepl > { b\x1B[12G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m\x1B[12G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m\x1B[12G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m \x1B[12G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }\x1B[12G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'\x1B[39m\x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b\x1B[39m\x1B[14G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m\x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m]\x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[13G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[36m=\x1B[39m\x1B[14G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[36m==\x1B[39m\x1B[14G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[36m===\x1B[39m\x1B[15G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[36m===\x1B[39m \x1B[14G\x1B[1G\x1B[0Jrepl > { b\x1B[36m:\x1B[39m \x1B[33m1\x1B[39m }[\x1B[32m'b'\x1B[39m] \x1B[36m===\x1B[39m \x1B[33m1\x1B[39m\x1B[14G\r",

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example

@avivkeller

Copy link
Copy Markdown
MemberAuthor

I no longer think this is a good idea. See the comment above.

@atlowChemi

Copy link
Copy Markdown
Member

and it'll clutter developers trying to simply parse the output

If that would be an issue, the NO_COLOR variable can be set etc, or we could add a util such as unstyleText.
I don't think this is a real issue, though this might be a semver-major

@avivkelleravivkeller reopened this Sep 21, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

If you say so, I trust your opinion. I'll keep this open in that case, though I have my reservations.

@aduh95aduh95 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 24, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Hey y'all, I don't have the time to persue this feature, given the other Node.js-related things I am working on. I'm closing this PR, but I'm happy for someone to continue the work.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.notable-changePRs with changes that should be highlighted in changelogs.replIssues and PRs related to the REPL subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@avivkeller@ZYSzys@BridgeAR@MoLow@atlowChemi@nodejs-github-bot@jasnell@aduh95