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

repl: accept no arguments to start() #5388

Merged
merged 1 commit into from
Feb 25, 2016
Merged

repl: accept no arguments to start() #5388

merged 1 commit into from
Feb 25, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 23, 2016

Currently, there is a check to ensure that the user either provides an object or a string to repl.start(). The string case is used to set a REPL prompt. However, a default of '> ' already exists, so forcing the user to specify a prompt is a bit redundant. This commit removes this restriction.

Closes #5385

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Feb 23, 2016
@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 23, 2016

What is the game plan for landing code changes while Jenkins is offline? Should we just wait until it's back to use the normal CI flow?

@bnoordhuis
Copy link
Member

I think that would be best.

@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 23, 2016
@Fishrock123
Copy link
Contributor

LGTM, we should be holding for CI I think.

@julianduque
Copy link
Contributor

LGTM with a minor nit, doc should be updated to specify options as optional argument: https://github.com/nodejs/node/blob/master/doc/api/repl.markdown#replstartoptions

Currently, there is a check to ensure that the user either
provides an object or a string to repl.start(). The string case
is used to set a REPL prompt. However, a default of '> ' already
exists, so forcing the user to specify a prompt is a bit
redundant. This commit removes this restriction.

Fixes: nodejs#5385
PR-URL: nodejs#5388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 25, 2016

Addressed @julianduque comment. CI is green - https://ci.nodejs.org/job/node-test-pull-request/1748/. Landing. Thanks for the reviews.

@cjihrig cjihrig merged commit ee7754b into nodejs:master Feb 25, 2016
@cjihrig cjihrig deleted the 5385 branch February 25, 2016 14:24
This was referenced Mar 1, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
Currently, there is a check to ensure that the user either
provides an object or a string to repl.start(). The string case
is used to set a REPL prompt. However, a default of '> ' already
exists, so forcing the user to specify a prompt is a bit
redundant. This commit removes this restriction.

Fixes: #5385
PR-URL: #5388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
Currently, there is a check to ensure that the user either
provides an object or a string to repl.start(). The string case
is used to set a REPL prompt. However, a default of '> ' already
exists, so forcing the user to specify a prompt is a bit
redundant. This commit removes this restriction.

Fixes: #5385
PR-URL: #5388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Fishrock123 added a commit that referenced this pull request Mar 8, 2016
Notable changes:

* child_process: “send()” now accepts an options parameter (cjihrig)
#5283
  - Currently the only option is “keepOpen”, which keeps the underlying
socket open after the message is sent.
* constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts)
#5463
* Fixed two regressions which originated in v5.7.0:
  - http: Errors inside of http client callbacks now propagate
correctly (Trevor Norris) #5591
  - path: Fixed normalization of absolute paths (Evan Lucas)
#5589
* repl: “start()” no longer requires an options parameter (cjihrig)
#5388
* util: Improved “format()” performance 50-300% (Evan Lucas)
#5360

PR-URL: #5559
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 9, 2016
Notable changes:

* child_process: “send()” now accepts an options parameter (cjihrig)
nodejs#5283
- Currently the only option is “keepOpen”, which keeps the underlying
socket open after the message is sent.
* constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts)
nodejs#5463
* Fixed two regressions which originated in v5.7.0:
  - http: Errors inside of http client callbacks now propagate
correctly (Trevor Norris) nodejs#5591
  - path: Fixed normalization of absolute paths (Evan Lucas)
nodejs#5589
* repl: “start()” no longer requires an options parameter (cjihrig)
nodejs#5388
* util: Improved “format()” performance 50-300% (Evan Lucas)
nodejs#5360

PR-URL: nodejs#5559
@Trott Trott mentioned this pull request May 18, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants