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.

Sign up for GitHub

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

Jump to bottom

repl: small refactoring #17919

Closed
wants to merge 3 commits into from
Closed

repl: small refactoring #17919

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd39e91
repl: upper case comments first char
BridgeAR Dec 30, 2017
9bf7fdb
repl: refactor code for readability
BridgeAR Dec 30, 2017
d8c95ca
fixup address comments
BridgeAR Jan 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 47 additions & 82 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const kBufferedCommandSymbol = Symbol('bufferedCommand');
const kContextId = Symbol('contextId');

try {
// hack for require.resolve("./relative") to work properly.
// Hack for require.resolve("./relative") to work properly.
module.filename = path.resolve('repl');
} catch (e) {
// path.resolve('repl') fails when the current working directory has been
Expand All @@ -90,7 +90,7 @@ try {
module.filename = path.resolve(dirname, 'repl');
}

// hack for repl require to work properly with node_modules folders
// Hack for repl require to work properly with node_modules folders
module.paths = Module._nodeModulePaths(module.filename);

// If obj.hasOwnProperty has been overridden, then calling
Expand All @@ -100,7 +100,6 @@ function hasOwnProperty(obj, prop) {
return Object.prototype.hasOwnProperty.call(obj, prop);
}


// Can overridden with custom print functions, such as `probe` or `eyes.js`.
// This is the default "writer" value if none is passed in the REPL options.
const writer = exports.writer = (obj) => util.inspect(obj, writer.options);
Expand All @@ -109,7 +108,6 @@ writer.options =

exports._builtinLibs = internalModule.builtinLibs;


function REPLServer(prompt,
stream,
eval_,
Expand Down Expand Up @@ -152,7 +150,6 @@ function REPLServer(prompt,
var self = this;

self._domain = dom || domain.create();

self.useGlobal = !!useGlobal;
self.ignoreUndefined = !!ignoreUndefined;
self.replMode = replMode || exports.REPL_MODE_SLOPPY;
Expand All @@ -163,7 +160,7 @@ function REPLServer(prompt,
// Context id for use with the inspector protocol.
self[kContextId] = undefined;

// just for backwards compat, see github.com/joyent/node/pull/7127
// Just for backwards compat, see github.com/joyent/node/pull/7127
self.rli = this;

const savedRegExMatches = ['', '', '', '', '', '', '', '', '', ''];
Expand Down Expand Up @@ -226,8 +223,7 @@ function REPLServer(prompt,
}
}

// first, create the Script object to check the syntax

// First, create the Script object to check the syntax
if (code === '\n')
return cb(null);

Expand All @@ -246,14 +242,14 @@ function REPLServer(prompt,
} catch (e) {
debug('parse error %j', code, e);
if (wrappedCmd) {
// unwrap and try again
// Unwrap and try again
wrappedCmd = false;
awaitPromise = false;
code = input;
wrappedErr = e;
continue;
}
// preserve original error for wrapped command
// Preserve original error for wrapped command
const error = wrappedErr || e;
if (isRecoverableError(error, code))
err = new Recoverable(error);
Expand Down Expand Up @@ -413,18 +409,13 @@ function REPLServer(prompt,
if (!input && !output) {
// legacy API, passing a 'stream'/'socket' option
if (!stream) {
// use stdin and stdout as the default streams if none were given
// Use stdin and stdout as the default streams if none were given
stream = process;
}
if (stream.stdin && stream.stdout) {
// We're given custom object with 2 streams, or the `process` object
input = stream.stdin;
output = stream.stdout;
} else {
// We're given a duplex readable/writable Stream, like a `net.Socket`
input = stream;
output = stream;
}
// We're given a duplex readable/writable Stream, like a `net.Socket`
// or a custom object with 2 streams, or the `process` object
input = stream.stdin || stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't make a difference, but this has the potential to be a behavior change if only one of stream.stdin or stream.stdout were passed in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, do you want me to change it to the way it was before? I personally do not feel like this is ever going to matter.

output = stream.stdout || stream;
}

self.inputStream = input;
Expand Down Expand Up @@ -463,7 +454,7 @@ function REPLServer(prompt,
this.commands = Object.create(null);
defineDefaultCommands(this);

// figure out which "writer" function to use
// Figure out which "writer" function to use
self.writer = options.writer || exports.writer;

if (options.useColors === undefined) {
Expand All @@ -478,14 +469,14 @@ function REPLServer(prompt,
}

function filterInternalStackFrames(error, structuredStack) {
// search from the bottom of the call stack to
// Search from the bottom of the call stack to
// find the first frame with a null function name
if (typeof structuredStack !== 'object')
return structuredStack;
const idx = structuredStack.reverse().findIndex(
(frame) => frame.getFunctionName() === null);

// if found, get rid of it and everything below it
// If found, get rid of it and everything below it
structuredStack = structuredStack.splice(idx + 1);
return structuredStack;
}
Expand Down Expand Up @@ -678,7 +669,7 @@ function REPLServer(prompt,
return;
}

// editor mode
// Editor mode
if (key.ctrl && !key.shift) {
switch (key.name) {
case 'd': // End editor mode
Expand All @@ -698,7 +689,7 @@ function REPLServer(prompt,
case 'down': // Override next history item
break;
case 'tab':
// prevent double tab behavior
// Prevent double tab behavior
self._previousKey = null;
ttyWrite(d, key);
break;
Expand Down Expand Up @@ -774,7 +765,7 @@ REPLServer.prototype.createContext = function() {
Object.defineProperty(context, 'console', {
configurable: true,
enumerable: true,
get: () => _console
value: _console
});

var names = Object.getOwnPropertyNames(global);
Expand Down Expand Up @@ -955,10 +946,8 @@ function complete(line, callback) {
}

var completions;

// list of completion lists, one for each inheritance "level"
// List of completion lists, one for each inheritance "level"
var completionGroups = [];

var completeOn, i, group, c;

// REPL commands (e.g. ".break").
Expand Down Expand Up @@ -1136,15 +1125,14 @@ function complete(line, callback) {
}
}
} catch (e) {
//console.log("completion error walking prototype chain:" + e);
// console.log("completion error walking prototype chain:" + e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}
}

if (memberGroups.length) {
for (i = 0; i < memberGroups.length; i++) {
completionGroups.push(memberGroups[i].map(function(member) {
return expr + '.' + member;
}));
completionGroups.push(
memberGroups[i].map((member) => expr + '.' + member));
Copy link

@vikkyconer vikkyconer Jan 1, 2018 •
edited by tniessen
Loading

Choose a reason for hiding this comment

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

const foo = memberGroups.reduce(( acc, memberGroup ) => {
    acc.push(memberGroup.map((member) => `${ expr }.${ member }`));
    return acc;
}, []);
completionGroups.concat(foo);

Don't mind the name foo as could not think of some descent name but this will make the code immutable. Although we are mutating completionGroups array but in next change we can convert it also to immutability

Copy link
Member Author

Choose a reason for hiding this comment

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

That is not necessary out of my perspective. I personally feel the current code is more expressive than using reduce.

}
if (filter) {
filter = expr + '.' + filter;
Copy link

@vikkyconer vikkyconer Jan 1, 2018 •
edited by tniessen
Loading

Choose a reason for hiding this comment

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

filter = expr + '.' + filter;

This can be changed to

filter = `${ expr }.${ filter }`;

Since it removes access usage of +

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Expand All @@ -1168,9 +1156,8 @@ function complete(line, callback) {
if (completionGroups.length && filter) {
var newCompletionGroups = [];
for (i = 0; i < completionGroups.length; i++) {
group = completionGroups[i].filter(function(elem) {
return elem.indexOf(filter) === 0;
});
group = completionGroups[i]
.filter((elem) => elem.indexOf(filter) === 0);
if (group.length) {
newCompletionGroups.push(group);
}
Expand All @@ -1179,7 +1166,7 @@ function complete(line, callback) {
}

if (completionGroups.length) {
var uniq = {}; // unique completions across all groups
var uniq = {}; // Unique completions across all groups
completions = [];
// Completion group 0 is the "closest"
// (least far up the inheritance chain)
Expand All @@ -1194,7 +1181,7 @@ function complete(line, callback) {
uniq[c] = true;
}
}
completions.push(''); // separator btwn groups
completions.push(''); // Separator btwn groups
}
while (completions.length && completions[completions.length - 1] === '') {
completions.pop();
Expand Down Expand Up @@ -1261,7 +1248,7 @@ function _memory(cmd) {
self.lines = self.lines || [];
self.lines.level = self.lines.level || [];

// save the line so I can do magic later
// Save the line so I can do magic later
if (cmd) {
// TODO should I tab the level?
const len = self.lines.level.length ? self.lines.level.length - 1 : 0;
Expand All @@ -1275,7 +1262,7 @@ function _memory(cmd) {
// Because I can not tell the difference between a } that
// closes an object literal and a } that closes a function
if (cmd) {
// going down is { and ( e.g. function() {
// Going down is { and ( e.g. function() {
// going up is } and )
var dw = cmd.match(/{|\(/g);
var up = cmd.match(/}|\)/g);
Expand All @@ -1286,8 +1273,8 @@ function _memory(cmd) {
if (depth) {
(function workIt() {
if (depth > 0) {
// going... down.
// push the line#, depth count, and if the line is a function.
// Going... down.
// Push the line#, depth count, and if the line is a function.
// Since JS only has functional scope I only need to remove
// "function() {" lines, clearly this will not work for
// "function()
Expand All @@ -1299,16 +1286,16 @@ function _memory(cmd) {
isFunction: /\bfunction\b/.test(cmd)
});
} else if (depth < 0) {
// going... up.
// Going... up.
var curr = self.lines.level.pop();
if (curr) {
var tmp = curr.depth + depth;
if (tmp < 0) {
//more to go, recurse
// More to go, recurse
depth += curr.depth;
workIt();
} else if (tmp > 0) {
//remove and push back
// Remove and push back
curr.depth += depth;
self.lines.level.push(curr);
}
Expand Down Expand Up @@ -1499,55 +1486,33 @@ function isCodeRecoverable(code) {

if (previous === '\\' && (stringLiteral || isRegExpLiteral)) {
current = null;
continue;
}

if (stringLiteral) {
} else if (stringLiteral) {
if (stringLiteral === current) {
stringLiteral = null;
}
continue;
} else {
if (isRegExpLiteral && current === '/') {
isRegExpLiteral = false;
continue;
}

if (isBlockComment && previous === '*' && current === '/') {
isBlockComment = false;
continue;
}

if (isSingleComment && current === '\n') {
isSingleComment = false;
continue;
}

if (isBlockComment || isRegExpLiteral || isSingleComment) continue;

} else if (isRegExpLiteral && current === '/') {
isRegExpLiteral = false;
} else if (isBlockComment && previous === '*' && current === '/') {
isBlockComment = false;
} else if (isSingleComment && current === '\n') {
isSingleComment = false;
} else if (!isBlockComment && !isRegExpLiteral && !isSingleComment) {
if (current === '/' && previous === '/') {
isSingleComment = true;
continue;
}

if (previous === '/') {
} else if (previous === '/') {
if (current === '*') {
isBlockComment = true;
} else if (
// Distinguish between a division operator and the start of a regex
// by examining the non-whitespace character that precedes the /
[null, '(', '[', '{', '}', ';'].includes(prevTokenChar)
) {
} else if ([null, '(', '[', '{', '}', ';'].includes(prevTokenChar)) {
isRegExpLiteral = true;
}
continue;
} else {
if (current.trim()) prevTokenChar = current;
if (current === '\'' || current === '"') {
stringLiteral = current;
}
}

if (current.trim()) prevTokenChar = current;
}

if (current === '\'' || current === '"') {
stringLiteral = current;
}
}

Expand Down