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

repl: refactor repl.js by Trott · Pull Request #6071 · nodejs/node · GitHub

/ node Public

repl: refactor repl.js - #6071

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:wip
ConversationCommits (1)ChecksFiles changed
Closed

repl: refactor repl.js#6071
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:wip

Conversation

Trott commented Apr 6, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

There is some unnecessary logic in repl.js. Remove it.

Trott added the repl Issues and PRs related to the REPL subsystem. label Apr 6, 2016

Fishrock123 commented Apr 6, 2016

Copy link
Copy Markdown
Contributor

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

Trott commented Apr 6, 2016

Copy link
Copy Markdown
Member Author

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

Trott mentioned this pull request Apr 6, 2016
Closed
4 tasks

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

LGTM

Fishrock123 reviewed Apr 7, 2016
View reviewed changes
Comment thread lib/repl.js Outdated

Fishrock123 Apr 7, 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

This won't have problems if a string doesn't get passed? If so, should the if (cmd) below be cmd !== ''?

Trott Apr 7, 2016

Copy link
Copy Markdown
Member 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

The existing code does not protect against non-strings. It only protects against falsy values.

You can't send a non-string without messing with REPLServer. I'll see about writing a test to make sure we are failing the same way when someone does that.

Trott Apr 7, 2016

Copy link
Copy Markdown
Member 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

Re-adding check for cmd, alas.

Trott added the test Issues and PRs related to the tests. label Apr 7, 2016
repl: refactor repl.js
00130b6 
There is some unnecessary logic in repl.js. Remove it.

Trott commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

Re-instated check for falsy cmd and added test. PTAL

Trott commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

CI is blissfully green.

Trott commented Apr 8, 2016

Copy link
Copy Markdown
Member Author

@jasnell: Still LGTY?

jasnell commented Apr 8, 2016

Copy link
Copy Markdown
Member

yep!

Trott added a commit to Trott/io.js that referenced this pull request Apr 8, 2016
repl: refactor repl.js
c5afd98 
There is some unnecessary logic in repl.js. Remove it.

PR-URL: nodejs#6071
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott commented Apr 8, 2016

Copy link
Copy Markdown
Member Author

Landed in c5afd98

Trott closed this Apr 8, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
repl: refactor repl.js
1162420 
There is some unnecessary logic in repl.js. Remove it.

PR-URL: #6071
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
repl: refactor repl.js
820dd58 
There is some unnecessary logic in repl.js. Remove it.

PR-URL: #6071
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Apr 20, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
repl: refactor repl.js
2254f1a 
There is some unnecessary logic in repl.js. Remove it.

PR-URL: #6071
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 21, 2016
Closed
Closed
jasnell pushed a commit that referenced this pull request Apr 26, 2016
repl: refactor repl.js
16f011e 
There is some unnecessary logic in repl.js. Remove it.

PR-URL: #6071
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented May 17, 2016

Copy link
Copy Markdown
Contributor

@Trott this was causing a test to fail on v4.x-staging. Would you be willing to backport?

santigimeno mentioned this pull request May 18, 2016
Closed
2 tasks

Trott commented May 18, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd I think this depends on #5388 which doesn't look like it has landed in the 4.x branch. If that's going to land in 4.x, land it first, then this should be fine (tests will pass, etc.). If that's not going to land in 4.x, then maybe this doesn't need to land in 4.x either.

MylesBorins commented May 18, 2016

Copy link
Copy Markdown
Contributor

Setting as do not land... thanks @Trott

addaleax reviewed May 28, 2016
View reviewed changes
Comment thread test/parallel/test-repl-null.js

var replserver = new repl.REPLServer();

replserver._inTemplateLiteral = true;

addaleax May 28, 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

@Trott do you still remember why you added this here?

Trott May 28, 2016

Copy link
Copy Markdown
Member 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

I remember why I did it, but I do not remember why I chose that over possible alternatives.

I did that because this is going to test that null inside a template string gets treated like an empty string. (See the code comment below.)

It's possible that I was being lazy and/or copying from another test. It may be better to open an actual template string by emiting a backtick in a line. Or it's possible that I tried that and ran into problems.

Trott May 28, 2016

Copy link
Copy Markdown
Member 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

Oh, and I was testing this particular corner case because I wanted to make sure that my refactoring the code didn't alter the behavior. And this seemed to me like odd behavior for which we had no tests.

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

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL