readline: add getPrompt to get the current prompt #33675

mattiasrunge · 2020-05-31T17:38:17Z

Since there is a setPrompt() there should be a getPrompt().
There are use-cases where it is needed to know what the
current prompt is. Adding a getPrompt() negates the need
to store the set prompt externally or read the internal
_prompt which would be bad practice.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/readline.md

benjamingr

This looks reasonable to me. I think supporting this makes sense.

Can you please:

Add a test (test/parallel/test-readline-interface.js is where I'd put it).
Change the 8 other places in the code that use ._prompt to .getPrompt() (lib/internal/util.js, test-readline-interface and test-repl-options ideally).

Also cc @BridgeAR

mattiasrunge · 2020-05-31T19:11:10Z

This looks reasonable to me. I think supporting this makes sense.

Can you please:

Add a test (test/parallel/test-readline-interface.js is where I'd put it).

Change the 8 other places in the code that use ._prompt to .getPrompt() (lib/internal/util.js, test-readline-interface and test-repl-options ideally).

Also cc @BridgeAR

Thank you for reviewing this so quickly! :)
Not sure exactly what you mean by adding a test. I have already added a test in the file you suggested. Do you want me to test something more?

I will try to find the places where _prompt is used currently and change.

benjamingr · 2020-05-31T19:12:39Z

I have already added a test in the file you suggested.

Sorry, I somehow missed that, the test looks good to me, just the usages and I want Ruben's take on this since it affects the REPL :]

benjamingr · 2020-06-01T20:14:22Z

@nodejs/readline

doc/api/readline.md

benjamingr

Making my LGTM explicit

BridgeAR

Too bad that the prompt() does not use a different name.

mattiasrunge · 2020-06-02T14:06:49Z

Anyone know anything about the tarball-failure? Does not seem connected to this change.

2020-06-01T21:08:57.0714361Z ##[section]Starting: Request a runner to run this job
2020-06-01T21:08:57.4754213Z Can't find any online and idle self-hosted runner in current repository that matches the required labels: 'macos-latest'
2020-06-01T21:08:57.4754260Z Can't find any online and idle self-hosted runner in current repository's account/organization that matches the required labels: 'macos-latest'
2020-06-01T21:08:57.4754294Z Found online and idle hosted runner in current repository's account/organization that matches the required labels: 'macos-latest'
2020-06-01T21:08:57.6616451Z ##[section]Finishing: Request a runner to run this job

jasnell

At some point we should runtime deprecate the public access to _prompt but that can definitely be done later.

mattiasrunge · 2020-10-14T18:18:03Z

Not sure how this is going, I did a rebase to keep it current. Is there a possibility to get this merged, I have seen no objections so far in the comments I've got.

benjamingr · 2020-10-15T08:23:57Z

@mattiasrunge I think this was just missed between other PRs (sorry! This is our fault!).

Any chance you could squash the commits into a single one so we can try landing this using the (automated) commit-queue rather than manually :]?

Since there is a setPrompt() there should be a getPrompt(). There are use-cases where it is needed to know what the current prompt is. Adding a getPrompt() negates the need to store the set prompt externally or read the internal _prompt which would be bad practice. Co-authored-by: Colin Ihrig <[email protected]>

mattiasrunge · 2020-10-19T06:54:40Z

Any chance you could squash the commits into a single one so we can try landing this using the (automated) commit-queue rather than manually :]?

@benjamingr, now there should only be one squashed commit.

nodejs-github-bot · 2020-11-09T17:40:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34247/

github-actions · 2020-11-10T09:48:13Z

Commit Queue failed

- Loading data for nodejs/node/pull/33675
✔  Done loading data for nodejs/node/pull/33675
----------------------------------- PR info ------------------------------------
Title      readline: add getPrompt to get the current prompt (#33675)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     mattiasrunge:readline-prompt-improve -> nodejs:master
Labels     author ready, readline
Commits    1
 - readline: add getPrompt to get the current prompt
Committers 1
 - Mattias Runge-Broberg 
PR-URL: https://github.com/nodejs/node/pull/33675
Reviewed-By: Colin Ihrig 
Reviewed-By: Anto Aravinth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/33675
Reviewed-By: Colin Ihrig 
Reviewed-By: Anto Aravinth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - readline: add getPrompt to get the current prompt
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-11-09T17:40:52Z: https://ci.nodejs.org/job/node-test-pull-request/34247/
- Querying data for job/node-test-pull-request/34247/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 31 May 2020 17:38:17 GMT
   ✔  Approvals: 6
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/33675#pullrequestreview-422280593
   ✔  - Anto Aravinth (@antsmartian): https://github.com/nodejs/node/pull/33675#pullrequestreview-422315730
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/33675#pullrequestreview-422469421
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/33675#pullrequestreview-422533984
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/33675#pullrequestreview-422547148
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/33675#pullrequestreview-422721331
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/355594986

benjamingr · 2020-11-10T12:55:20Z

I'll resume CI and try landing with it

nodejs-github-bot · 2020-11-10T12:55:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34284/

juanarbol

This is so goood, thank you 🎉

github-actions · 2020-11-10T19:46:55Z

Landed in 3c38445...60a97c0

Since there is a setPrompt() there should be a getPrompt(). There are use-cases where it is needed to know what the current prompt is. Adding a getPrompt() negates the need to store the set prompt externally or read the internal _prompt which would be bad practice. Co-authored-by: Colin Ihrig <[email protected]> PR-URL: #33675 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

benjamingr · 2020-11-10T19:48:18Z

Thank you for your contribution @mattiasrunge and congratulations on your first contribution 🎉

mattiasrunge · 2020-11-10T19:52:31Z

Woho! Thank you @benjamingr!

Since there is a setPrompt() there should be a getPrompt(). There are use-cases where it is needed to know what the current prompt is. Adding a getPrompt() negates the need to store the set prompt externally or read the internal _prompt which would be bad practice. Co-authored-by: Colin Ihrig <[email protected]> PR-URL: #33675 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Notable changes: dns: * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099 events: * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001 http: * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048 http2: * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 lib: * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716 path: * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962 readline: * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675 src: * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940 util: * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055 PR-URL: TODO

Notable changes: dns: * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099 events: * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001 http: * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048 http2: * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 lib: * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716 path: * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962 readline: * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675 src: * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940 util: * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055 PR-URL: #36232

Since there is a setPrompt() there should be a getPrompt(). There are use-cases where it is needed to know what the current prompt is. Adding a getPrompt() negates the need to store the set prompt externally or read the internal _prompt which would be bad practice. Co-authored-by: Colin Ihrig <[email protected]> PR-URL: #33675 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label May 31, 2020

mscdex reviewed May 31, 2020

View reviewed changes

doc/api/readline.md Outdated Show resolved Hide resolved

mscdex reviewed May 31, 2020

View reviewed changes

doc/api/readline.md Show resolved Hide resolved

benjamingr reviewed May 31, 2020

View reviewed changes

cjihrig reviewed Jun 1, 2020

View reviewed changes

doc/api/readline.md Outdated Show resolved Hide resolved

cjihrig approved these changes Jun 2, 2020

View reviewed changes

antsmartian approved these changes Jun 2, 2020

View reviewed changes

benjamingr approved these changes Jun 2, 2020

View reviewed changes

BridgeAR approved these changes Jun 2, 2020

View reviewed changes

targos approved these changes Jun 2, 2020

View reviewed changes

jasnell approved these changes Jun 2, 2020

View reviewed changes

mattiasrunge force-pushed the readline-prompt-improve branch from 59b2c02 to b17a0e6 Compare June 23, 2020 13:31

mattiasrunge force-pushed the readline-prompt-improve branch from b17a0e6 to 682f17f Compare October 14, 2020 18:16

mattiasrunge force-pushed the readline-prompt-improve branch from 682f17f to f315b21 Compare October 19, 2020 06:52

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 8, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020

This comment has been minimized.

Sign in to view

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 10, 2020

juanarbol approved these changes Nov 10, 2020

View reviewed changes

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2020

github-actions bot closed this Nov 10, 2020

codebytere mentioned this pull request Nov 22, 2020

v15.3.0 proposal #36232

Merged

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: add getPrompt to get the current prompt #33675

readline: add getPrompt to get the current prompt #33675

mattiasrunge commented May 31, 2020

benjamingr left a comment

mattiasrunge commented May 31, 2020

benjamingr commented May 31, 2020

benjamingr commented Jun 1, 2020

benjamingr left a comment

BridgeAR left a comment

mattiasrunge commented Jun 2, 2020

jasnell left a comment

mattiasrunge commented Oct 14, 2020

benjamingr commented Oct 15, 2020

mattiasrunge commented Oct 19, 2020

This comment has been minimized.

nodejs-github-bot commented Nov 9, 2020

github-actions bot commented Nov 10, 2020

benjamingr commented Nov 10, 2020

nodejs-github-bot commented Nov 10, 2020

juanarbol left a comment

github-actions bot commented Nov 10, 2020

benjamingr commented Nov 10, 2020

mattiasrunge commented Nov 10, 2020

readline: add getPrompt to get the current prompt #33675

readline: add getPrompt to get the current prompt #33675

Conversation

mattiasrunge commented May 31, 2020

Checklist

benjamingr left a comment

Choose a reason for hiding this comment

mattiasrunge commented May 31, 2020

benjamingr commented May 31, 2020

benjamingr commented Jun 1, 2020

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

mattiasrunge commented Jun 2, 2020

jasnell left a comment

Choose a reason for hiding this comment

mattiasrunge commented Oct 14, 2020

benjamingr commented Oct 15, 2020

mattiasrunge commented Oct 19, 2020

This comment has been minimized.

nodejs-github-bot commented Nov 9, 2020

github-actions bot commented Nov 10, 2020

benjamingr commented Nov 10, 2020

nodejs-github-bot commented Nov 10, 2020

juanarbol left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2020

benjamingr commented Nov 10, 2020

mattiasrunge commented Nov 10, 2020