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 <cjihrig@gmail.com>

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 <cjihrig@gmail.com> PR-URL: #33675 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

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 <cjihrig@gmail.com> PR-URL: #33675 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

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 <cjihrig@gmail.com> PR-URL: #33675 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

nodejs-github-bot added the readline label May 31, 2020

mscdex reviewed May 31, 2020

View changes

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

mscdex reviewed May 31, 2020

View changes

doc/api/readline.md Show resolved Hide resolved

benjamingr reviewed May 31, 2020

View changes

cjihrig reviewed Jun 1, 2020

View changes

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

cjihrig approved these changes Jun 2, 2020

View changes

antsmartian approved these changes Jun 2, 2020

View changes

benjamingr approved these changes Jun 2, 2020

View changes

BridgeAR approved these changes Jun 2, 2020

View changes

targos approved these changes Jun 2, 2020

View changes

jasnell approved these changes Jun 2, 2020

View changes

mattiasrunge force-pushed the readline-prompt-improve branch from 59b2c02 to b17a0e6 Compare Jun 23, 2020

mattiasrunge force-pushed the readline-prompt-improve branch from b17a0e6 to 682f17f Compare Oct 14, 2020

mattiasrunge force-pushed the readline-prompt-improve branch from 682f17f to f315b21 Compare Oct 19, 2020

aduh95 added author ready request-ci labels Nov 8, 2020

github-actions bot removed the request-ci label Nov 8, 2020

This comment has been minimized.

Sign in to view

aduh95 added the commit-queue label Nov 10, 2020

github-actions bot added commit-queue-failed and removed commit-queue labels Nov 10, 2020

juanarbol approved these changes Nov 10, 2020

View changes

benjamingr added commit-queue semver-minor and removed commit-queue-failed labels Nov 10, 2020

github-actions bot removed the commit-queue 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

nodejs / node Public

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

Mar	APR	May
	23
2021	2022	2023

nodejs / node Public

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

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