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

readline: inform line event about separator used #23929

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 20 additions & 10 deletions lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ let StringDecoder;
const kHistorySize = 30;
const kMincrlfDelay = 100;
// \r\n, \n, or \r followed by something other than \n
const lineEnding = /\r?\n|\r(?!\n)/;
const lineEndingWithSeparator = /(?<=\r?\n|\r(?!\n))/;
const lineEndingSeparatorInclusion = /[\r?\n|\r(?!\n)]$/;
Copy link
Member

Choose a reason for hiding this comment

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

The regular expression seems to be faulty. Wrapping the values in the square brackets means that each character is checked for and the special characters loose their meaning and will be checked for as well.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the insight, I'll check and update


const KEYPRESS_DECODER = Symbol('keypress-decoder');
const ESCAPE_DECODER = Symbol('escape-decoder');
Expand Down Expand Up @@ -290,14 +291,14 @@ Interface.prototype.question = function(query, cb) {
};


Interface.prototype._onLine = function(line) {
Interface.prototype._onLine = function(line, separator = '\n') {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the default value here? Due to your changes it seems like the value is always set?

Copy link
Author

Choose a reason for hiding this comment

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

@BridgeAR Hey thanks for taking time for the review. I've mostly passed around the appropriate values in the argument in the places I saw this function was being invoked. The idea behind the default value is to not alter its behavior if its called with falsy values by mistake by any call site.

if (this._questionCallback) {
var cb = this._questionCallback;
this._questionCallback = null;
this.setPrompt(this._oldPrompt);
cb(line);
} else {
this.emit('line', line);
this.emit('line', line, separator);
}
};

Expand Down Expand Up @@ -425,7 +426,7 @@ Interface.prototype._normalWrite = function(b) {
}

// Run test() on the new string chunk, not on the entire line buffer.
var newPartContainsEnding = lineEnding.test(string);
var newPartContainsEnding = lineEndingWithSeparator.test(string);

if (this._line_buffer) {
string = this._line_buffer + string;
Expand All @@ -435,12 +436,21 @@ Interface.prototype._normalWrite = function(b) {
this._sawReturnAt = string.endsWith('\r') ? Date.now() : 0;

// got one or more newlines; process into "line" events
var lines = string.split(lineEnding);
var lines = string.split(lineEndingWithSeparator);
// either '' or (conceivably) the unfinished portion of the next line
string = lines.pop();
this._line_buffer = string;
for (var n = 0; n < lines.length; n++)
this._onLine(lines[n]);
for (var n = 0; n < lines.length; n++) {
const _line = lines[n];
if (lineEndingSeparatorInclusion.test(_line)) {
const lineWithoutSeparator = _line.slice(0, -1);
const separator = _line.slice(-1);

this._onLine(lineWithoutSeparator, separator);
} else {
this._onLine(_line, '\n');
}
Copy link
Member

Choose a reason for hiding this comment

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

If I am not mistaken, each line has a separator here and the else case will not be reached besides potentially for the very last line. Instead of checking with a regular expression what is used, it would be significantly faster to just check the last two characters of each line. But in that case the line would still require to be sliced and that is extra overhead. It's probably faster to use a loop that checks for some special characters and to manually slice reach line.

}
} else if (string) {
// no newlines this time, save what we have for next time
this._line_buffer = string;
Expand Down Expand Up @@ -642,10 +652,10 @@ Interface.prototype.clearLine = function() {
};


Interface.prototype._line = function() {
Interface.prototype._line = function(separator = '\n') {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest not to use a default value here and to be explicit in the call sites. Especially the call site that uses s.split(/\r\n|\n|\r/). In that case it would be best to make sure these are kept as before as well.

Copy link
Author

Choose a reason for hiding this comment

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

no problem, I'll revert the change. I was just being cautious about the bugs that might happen later if someone forgets to add the separator.

var line = this._addHistory();
this.clearLine();
this._onLine(line);
this._onLine(line, separator);
};


Expand Down Expand Up @@ -924,7 +934,7 @@ Interface.prototype._ttyWrite = function(s, key) {
switch (key.name) {
case 'return': // carriage return, i.e. \r
this._sawReturnAt = Date.now();
this._line();
this._line('\r');
break;

case 'enter':
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-readline-interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,10 @@ function isWarned(emitter) {
{ input: fi, output: fi, terminal: terminal }
);
let called = false;
rli.on('line', function(line) {
rli.on('line', function(line, separator) {
called = true;
assert.strictEqual(line, 'asdf');
assert.strictEqual(separator, '\n');
});
fi.emit('data', 'asdf\n');
assert.ok(called);
Expand Down