readline: allow passing prompt to constructor #7125

evanlucas · 2016-06-03T01:34:44Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

readline

Description of change

Previously, one would have to call setPrompt after calling
rl.createInterface(). Now, it the prompt string can be set by passing the
prompt property.

evanlucas · 2016-06-03T01:37:08Z

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

cjihrig · 2016-06-03T02:00:10Z

test/parallel/test-readline-interface.js

+  });
+
+  r = readline.createInterface({
+    input: new Readable({ read: () => {}}),


There is uneven spacing inside the first set of curly braces.

cjihrig · 2016-06-03T02:02:27Z

LGTM with one comment.

Fishrock123 · 2016-06-03T21:02:26Z

test/parallel/test-readline-interface.js

@@ -396,4 +398,28 @@ function isWarned(emitter) {
    });
  });

+  var r;


use more descriptive variables within a { } block to prevent leakage into other future test cases?

Fishrock123 · 2016-06-03T21:03:29Z

Should the repl constructor now set it directly via this?

evanlucas · 2016-06-03T22:56:45Z

@Fishrock123 good call. Should that be in a separate commit or this one?

Fishrock123 · 2016-06-04T13:47:45Z

@evanlucas Either or, probably belongs in the same PR

jasnell · 2016-06-06T15:50:15Z

might be worthwhile updating examples in the doc to show use of this. LGTM

evanlucas · 2016-06-06T23:46:12Z

Updated to address comments. PTAL

evanlucas · 2016-06-10T11:42:33Z

CI again after rebasing: https://ci.nodejs.org/job/node-test-pull-request/2973/

evanlucas · 2016-06-18T12:54:33Z

@jasnell @cjihrig @Fishrock123 LGTY?

cjihrig · 2016-06-20T15:05:47Z

lib/repl.js

@@ -344,11 +344,10 @@ function REPLServer(prompt,
    output: self.outputStream,
    completer: complete,
    terminal: options.terminal,
-    historySize: options.historySize
+    historySize: options.historySize,
+    prompt: prompt


Not sure that it matters, but this could just be prompt

cjihrig · 2016-06-20T15:10:30Z

LGTM

evanlucas · 2016-06-22T12:33:54Z

CI one more time https://ci.nodejs.org/job/node-test-pull-request/3050/

evanlucas · 2016-06-23T02:56:59Z

trying one more time https://ci.nodejs.org/job/node-test-pull-request/3059/

evanlucas · 2016-06-23T13:18:37Z

How are we handling the added information for things that are not methods added, but like this, an option added. Should I add an added label under prompt?

cjihrig · 2016-06-23T13:26:27Z

Good question. It doesn't look like we are doing anything like that yet.

evanlucas · 2016-06-23T13:27:30Z

I think it makes sense to somehow note the version it was added, just not sure it makes sense to do it in the same way as a method introduction.

cjihrig · 2016-06-23T13:51:53Z

I agree, that information is really useful to have. Any ideas from @nodejs/documentation?

addaleax · 2016-06-23T14:47:06Z

Adding some sort of changelog for individual methods is definitely planned after #6578, yep.

Fishrock123 · 2016-06-24T11:48:35Z

doc/api/readline.md

@@ -357,6 +357,7 @@ added: v0.1.98
    the history set this value to `0`. Defaults to `30`. This option makes sense
    only if `terminal` is set to `true` by the user or by an internal `output`
    check, otherwise the history caching mechanism is not initialized at all.
+  * `prompt` - the prompt string to use.


Default: `'> '`

Fishrock123 · 2016-06-24T11:48:47Z

LGTM minus my and @cjihrig 's nits

evanlucas · 2016-06-25T13:57:48Z

Alright, nits fixed, one more ci before landing https://ci.nodejs.org/job/node-test-pull-request/3083/

Previously, one would have to call setPrompt after calling rl.createInterface. Now, the prompt string can be set by passing the prompt property. PR-URL: nodejs#7125 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

evanlucas · 2016-06-25T17:11:14Z

Landed in 3f5623d. Thanks!

Previously, one would have to call setPrompt after calling rl.createInterface. Now, the prompt string can be set by passing the prompt property. PR-URL: #7125 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Previously, one would have to call setPrompt after calling rl.createInterface. Now, the prompt string can be set by passing the prompt property. PR-URL: #7125 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Conflicts: test/parallel/test-readline-interface.js

Notable changes: * buffer: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) #7157 * build: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) #6994 - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * crypto: Root certificates have been updated. (Ben Noordhuis) #7363 * debugger: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) #3316 * npm: Upgraded npm to v3.10.3 (Kat Marchán) #7515 & (Rebecca Turner) #7410 * readline: Added the `prompt` option to the readline constructor. (Evan Lucas) #7125 * repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) #6635 * src: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) #3098 - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) #6534 * stream: Improved `readable.read()` performance by up to 70%. (Brian White) #7077 * timers: `setImmediate()` is now up to 150% faster in some situations. (Andras) #6436 * util: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) #7499 * v8-inspector: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) #6792 - *Note: This feature is experimental, and it could be altered or removed.* - You can try this feature by running Node.js with the `--inspect` flag. Refs: #7441 PR-URL: #7550

### Notable changes * **buffer**: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) [#7157](nodejs/node#7157) * **build**: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) [#6994](nodejs/node#6994) - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * **crypto**: Root certificates have been updated. (Ben Noordhuis) [#7363](nodejs/node#7363) * **debugger**: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) [#3316](nodejs/node#3316) * **npm**: Upgraded npm to v3.10.3 (Kat Marchán) [#7515](nodejs/node#7515) & (Rebecca Turner) [#7410](nodejs/node#7410) * **readline**: Added the `prompt` option to the readline constructor. (Evan Lucas) [#7125](nodejs/node#7125) * **repl / vm**: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) [#6635](nodejs/node#6635) * **src**: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) [#3098](nodejs/node#3098) - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) [#6534](nodejs/node#6534) * **stream**: Improved `readable.read()` performance by up to 70%. (Brian White) [#7077](nodejs/node#7077) * **timers**: `setImmediate()` is now up to 150% faster in some situations. (Andras) [#6436](nodejs/node#6436) * **util**: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) [#7499](nodejs/node#7499) * **v8-inspector**: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) [#6792](nodejs/node#6792) - **Note: This feature is _experimental_, and it could be altered or removed.** - You can try this feature by running Node.js with the `--inspect` flag.

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 3, 2016

evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 3, 2016

cjihrig reviewed Jun 3, 2016
View reviewed changes

evanlucas force-pushed the rlprompt branch from 8cfe9a2 to 07ffa16 Compare June 3, 2016 02:03

Fishrock123 reviewed Jun 3, 2016
View reviewed changes

evanlucas force-pushed the rlprompt branch from 07ffa16 to a54babe Compare June 6, 2016 23:45

evanlucas force-pushed the rlprompt branch from a54babe to c6aa0c4 Compare June 10, 2016 11:40

cjihrig reviewed Jun 20, 2016
View reviewed changes

Fishrock123 reviewed Jun 24, 2016
View reviewed changes

evanlucas force-pushed the rlprompt branch from c6aa0c4 to 0ad080f Compare June 24, 2016 22:23

evanlucas force-pushed the rlprompt branch from 0ad080f to 3f5623d Compare June 25, 2016 17:10

evanlucas closed this Jun 25, 2016

evanlucas deleted the rlprompt branch June 25, 2016 17:10

evanlucas merged commit 3f5623d into nodejs:master Jun 25, 2016

Fishrock123 mentioned this pull request Jun 27, 2016

Propose v6.3.0 #7441

Merged

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

cjean-fr mentioned this pull request Jul 7, 2016

Update nodejs to 6.3.0 ScoopInstaller/Scoop#948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: allow passing prompt to constructor #7125

readline: allow passing prompt to constructor #7125

evanlucas commented Jun 3, 2016

evanlucas commented Jun 3, 2016

cjihrig Jun 3, 2016

cjihrig commented Jun 3, 2016

Fishrock123 Jun 3, 2016

evanlucas Jun 3, 2016

Fishrock123 commented Jun 3, 2016

evanlucas commented Jun 3, 2016

Fishrock123 commented Jun 4, 2016

jasnell commented Jun 6, 2016

evanlucas commented Jun 6, 2016

evanlucas commented Jun 10, 2016

evanlucas commented Jun 18, 2016

cjihrig Jun 20, 2016

cjihrig commented Jun 20, 2016

evanlucas commented Jun 22, 2016

evanlucas commented Jun 23, 2016

evanlucas commented Jun 23, 2016

cjihrig commented Jun 23, 2016

evanlucas commented Jun 23, 2016

cjihrig commented Jun 23, 2016

addaleax commented Jun 23, 2016

Fishrock123 Jun 24, 2016

Fishrock123 commented Jun 24, 2016

evanlucas commented Jun 25, 2016

evanlucas commented Jun 25, 2016

@@ @@ -396,4 +398,28 @@ function isWarned(emitter) { @@
                   });
                 });
+                var r;

readline: allow passing prompt to constructor #7125

readline: allow passing prompt to constructor #7125

Conversation

evanlucas commented Jun 3, 2016

Checklist

Affected core subsystem(s)

Description of change

evanlucas commented Jun 3, 2016

cjihrig Jun 3, 2016

Choose a reason for hiding this comment

cjihrig commented Jun 3, 2016

Fishrock123 Jun 3, 2016

Choose a reason for hiding this comment

evanlucas Jun 3, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 3, 2016

evanlucas commented Jun 3, 2016

Fishrock123 commented Jun 4, 2016

jasnell commented Jun 6, 2016

evanlucas commented Jun 6, 2016

evanlucas commented Jun 10, 2016

evanlucas commented Jun 18, 2016

cjihrig Jun 20, 2016

Choose a reason for hiding this comment

cjihrig commented Jun 20, 2016

evanlucas commented Jun 22, 2016

evanlucas commented Jun 23, 2016

evanlucas commented Jun 23, 2016

cjihrig commented Jun 23, 2016

evanlucas commented Jun 23, 2016

cjihrig commented Jun 23, 2016

addaleax commented Jun 23, 2016

Fishrock123 Jun 24, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 24, 2016

evanlucas commented Jun 25, 2016

evanlucas commented Jun 25, 2016