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.

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

readline: allow passing prompt to constructor #7125

Merged
merged 1 commit into from
Jun 25, 2016

Conversation

evanlucas
Copy link
Contributor

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.

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 3, 2016
@evanlucas evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 3, 2016
@evanlucas
Copy link
Contributor Author

});

r = readline.createInterface({
input: new Readable({ read: () => {}}),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

cjihrig commented Jun 3, 2016

LGTM with one comment.

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

var r;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@Fishrock123
Copy link
Contributor

Should the repl constructor now set it directly via this?

@evanlucas
Copy link
Contributor Author

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

@Fishrock123
Copy link
Contributor

@evanlucas Either or, probably belongs in the same PR

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

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

@evanlucas
Copy link
Contributor Author

Updated to address comments. PTAL

@evanlucas
Copy link
Contributor Author

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

@evanlucas
Copy link
Contributor Author

@jasnell @cjihrig @Fishrock123 LGTY?

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

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

cjihrig commented Jun 20, 2016

LGTM

@evanlucas
Copy link
Contributor Author

@evanlucas
Copy link
Contributor Author

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

@evanlucas
Copy link
Contributor Author

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
Copy link
Contributor

cjihrig commented Jun 23, 2016

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

@evanlucas
Copy link
Contributor Author

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
Copy link
Contributor

cjihrig commented Jun 23, 2016

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

@addaleax
Copy link
Member

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default: `'> '`

@Fishrock123
Copy link
Contributor

LGTM minus my and @cjihrig 's nits

@evanlucas
Copy link
Contributor Author

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 evanlucas closed this Jun 25, 2016
@evanlucas evanlucas deleted the rlprompt branch June 25, 2016 17:10
@evanlucas evanlucas merged commit 3f5623d into nodejs:master Jun 25, 2016
@evanlucas
Copy link
Contributor Author

Landed in 3f5623d. Thanks!

Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
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>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
lukesampson pushed a commit to ScoopInstaller/Scoop that referenced this pull request Jul 7, 2016
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants