FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

readline: refactor construct Interface by JacksonTian · Pull Request #4740 · nodejs/node · GitHub

/ node Public

readline: refactor construct Interface - #4740

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:readline
ConversationCommits (1)ChecksFiles changed
Closed

readline: refactor construct Interface#4740
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:readline

Conversation

JacksonTian commented Jan 18, 2016

Copy link
Copy Markdown
Contributor

Remove the dependency on the arguments.length.

silverwind added the readline Issues and PRs related to the built-in readline module. label Jan 18, 2016

jasnell commented Jan 18, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Jan 18, 2016

Copy link
Copy Markdown
Member

I hadn't noticed the Interface constructor before... what an odd way of initializing the object... this change looks good but let's see what CI says :-)

cjihrig commented Jan 18, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Jan 22, 2016

Copy link
Copy Markdown
Member

CI failures look unrelated. LGTM

TooTallNate commented Jan 22, 2016

Copy link
Copy Markdown
Contributor

I'm pretty sure it was related to backwards compat with a really old node version (0.6?). Definitely safe to remove at this point.

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

MylesBorins commented Jun 17, 2016

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/3021/ (rebased against master)

@nodejs/collaborators PTAL

MylesBorins self-assigned this Jun 17, 2016

cjihrig commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

LGTM. Let's try another CI: https://ci.nodejs.org/job/node-test-pull-request/3034/

MylesBorins commented Jul 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

one more CI. Will land later today if green https://ci.nodejs.org/job/node-test-pull-request/3178/

rmg reviewed Jul 5, 2016
View reviewed changes
Comment thread lib/readline.js Outdated

rmg Jul 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this change in behaviour intentional? If input is a custom implementation that happens to have an input property, then it will cause the remaining arguments to be ignored. Can we find a safer way replicating the arguments length check? Maybe the ugly/obvious if (input && !output && !completer && !terminal) approach would be the most appropriate?

MylesBorins Jul 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thanks @rmg for scoping that out

thoughts @nodejs/collaborators?

RReverser Jul 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hmm, why not just keep arguments.length check?

rmg Jul 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

arguments.length wouldn't work with the way createInterface() was refactored - it now always passes 4 args, even if they're undefined.

RReverser Jul 6, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How about changing it then to

exports.createInterface = (...args) => new Interface(...args);

?

That should solve the problem and, at the same time, still looks clean and elegant IMO.

RReverser Jul 6, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same should be applied for the constructor though, but not hard to do.

rmg Jul 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

facepalm
this is targeting master, so I guess that's an option. Would also match lib/console.js, now that I look.

RReverser Jul 6, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In fact, given that Interface allows calling itself without new (and unless I'm missing something), it might be even reduced to exports.createInterface = Interface; (in which case I don't really see why two separate methods are exported, unless we want to move Interface to proper ES6 class and effectively break compatibility by forbidding calls without new in future).

JacksonTian Dec 12, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

no. It doesn't change the behaviour. if (input && input.input) { is used for check first argument is options object.

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
readline: refactor construct Interface
c4a12c8 
Remove the dependency on the arguments.length.
jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017

jasnell commented Jan 6, 2017

Copy link
Copy Markdown
Member

@MylesBorins @JacksonTian ... still want this? Looks like it fell through the cracks

JacksonTian commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

Yes.

JacksonTian commented Jan 13, 2017

Copy link
Copy Markdown
Contributor Author

landed in dad98bf thanks all.

JacksonTian added a commit that referenced this pull request Jan 13, 2017
readline: refactor construct Interface
dad98bf 
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
readline: refactor construct Interface
ba4aa80 
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
readline: refactor construct Interface
b987cae 
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
readline: refactor construct Interface
05fa225 
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
readline: refactor construct Interface
d64e237 
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
readline: refactor construct Interface
9aeb34a 
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
readline: refactor construct Interface
eea8abb 
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins commented Mar 8, 2017

Copy link
Copy Markdown
Contributor

I've gone ahead and landed this into lts staging. Please let me know if it should be taken out

MylesBorins added land-on-v4.x and removed lts-watch-v4.x stalled Issues and PRs that are stalled. labels Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
readline: refactor construct Interface
dc88b65 
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
readline: refactor construct Interface
396688f 
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
+1 more reviewer
trevnorris trevnorris approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

MylesBorins

Labels

readline Issues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL