-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)]$/; | ||
|
||
const KEYPRESS_DECODER = Symbol('keypress-decoder'); | ||
const ESCAPE_DECODER = Symbol('escape-decoder'); | ||
|
@@ -290,14 +291,14 @@ Interface.prototype.question = function(query, cb) { | |
}; | ||
|
||
|
||
Interface.prototype._onLine = function(line) { | ||
Interface.prototype._onLine = function(line, separator = '\n') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
}; | ||
|
||
|
@@ -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; | ||
|
@@ -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'); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -642,10 +652,10 @@ Interface.prototype.clearLine = function() { | |
}; | ||
|
||
|
||
Interface.prototype._line = function() { | ||
Interface.prototype._line = function(separator = '\n') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}; | ||
|
||
|
||
|
@@ -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': | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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