repl: refactor repl.js #6071
repl: refactor repl.js #6071
Conversation
|
LGTM |
| @@ -409,7 +408,7 @@ function REPLServer(prompt, | |||
|
|
|||
| // Check to see if a REPL keyword was used. If it returns true, | |||
| // display next prompt and return. | |||
| if (cmd && cmd.charAt(0) === '.' && isNaN(parseFloat(cmd))) { | |||
| if (cmd.charAt(0) === '.' && isNaN(parseFloat(cmd))) { | |||
There was a problem hiding this comment.
This won't have problems if a string doesn't get passed? If so, should the if (cmd) below be cmd !== ''?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Re-adding check for cmd, alas.
There is some unnecessary logic in repl.js. Remove it.
|
Re-instated check for falsy |
|
CI is blissfully green. |
|
@jasnell: Still LGTY? |
|
yep! |
There is some unnecessary logic in repl.js. Remove it. PR-URL: nodejs#6071 Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in c5afd98 |
There is some unnecessary logic in repl.js. Remove it. PR-URL: #6071 Reviewed-By: James M Snell <jasnell@gmail.com>
There is some unnecessary logic in repl.js. Remove it. PR-URL: #6071 Reviewed-By: James M Snell <jasnell@gmail.com>
There is some unnecessary logic in repl.js. Remove it. PR-URL: #6071 Reviewed-By: James M Snell <jasnell@gmail.com>
There is some unnecessary logic in repl.js. Remove it. PR-URL: #6071 Reviewed-By: James M Snell <jasnell@gmail.com>
|
@Trott this was causing a test to fail on v4.x-staging. Would you be willing to backport? |
|
@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. |
|
Setting as do not land... thanks @Trott |
|
|
||
| var replserver = new repl.REPLServer(); | ||
|
|
||
| replserver._inTemplateLiteral = true; |
There was a problem hiding this comment.
@Trott do you still remember why you added this here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Checklist
Affected core subsystem(s)
repl
Description of change
There is some unnecessary logic in repl.js. Remove it.