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.

Sign up for GitHub

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

Jump to bottom

repl: support syntax highlighting #53571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

repl: support syntax highlighting #53571

wants to merge 1 commit into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Jun 24, 2024 •
edited
Loading

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-bot nodejs-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
@RedYetiDev RedYetiDev changed the title repl: enable syntax highlighting Jun 24, 2024
@RedYetiDev
Copy link
Member Author

CC @nodejs/repl
@RedYetiDev RedYetiDev mentioned this pull request Jun 24, 2024
Closed
3 tasks
@ZYSzys
Copy link
Member

ZYSzys commented Jun 25, 2024

Could you add some tests ?
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 2, 2024 •
edited
Loading

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

The failed tests appear unrelated.
@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label Jul 9, 2024
atlowChemi
atlowChemi reviewed Jul 10, 2024
View reviewed changes
test/parallel/test-repl-syntax-highlighting.js Outdated Show resolved Hide resolved
lib/repl.js Show resolved Hide resolved
lib/internal/repl/highlight.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev removed the review wanted PRs that need reviews. label Jul 10, 2024
@RedYetiDev
Copy link
Member Author

(got review - removing label)
@RedYetiDev
Copy link
Member Author

@atlowChemi

All review conversations have been resolved, any more notes?
@BridgeAR
Copy link
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 :)
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 11, 2024 •
edited
Loading

@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
Member

MoLow commented Jul 14, 2024

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

I think both :)
@RedYetiDev RedYetiDev 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 GitHub Actions
Copy link
Contributor

The notable-change PRs 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.
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 14, 2024 •
edited
Loading

@MoLow ive added both labels.
@RedYetiDev
Copy link
Member Author

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 :-)!
atlowChemi
atlowChemi reviewed Jul 15, 2024
View reviewed changes
Copy link
Member

@atlowChemi atlowChemi left a comment

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.
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 16, 2024 •
edited
Loading

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
6 participants
@RedYetiDev @ZYSzys @BridgeAR @MoLow @atlowChemi @nodejs-github-bot