readline: inform line event about separator used #23929
Conversation
nodejs-github-bot
added
the
readline
|
to run the test use |
pass separator as a parameter to the callback alongwith line to inform line event about separator used to break lines fixes: nodejs#7952 (comment)
Sayanc93
force-pushed
the
inform-line-event-on-readline-about-endline-input
branch
from
9887ce2 to
687cdf5
Compare
|
@nodejs/repl This could use some reviews. |
| @@ -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.
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.
@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.
| @@ -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)]$/; | |||
There was a problem hiding this comment.
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.
thanks for the insight, I'll check and update
| @@ -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.
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.
There was a problem hiding this comment.
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.
| this._onLine(lineWithoutSeparator, separator); | ||
| } else { | ||
| this._onLine(_line, '\n'); | ||
| } |
There was a problem hiding this comment.
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.
|
It would be great to add a test with a file that contains mixed line endings. |
|
@BridgeAR Thanks for the review, this was a WIP branch, got caught up with other stuff. I'll push the changes with your suggestions soon. |
Trott
added
the
semver-minor
|
Is this still being worked on? |
|
Ping @Sayanc93 |
|
That seems pretty good, it please let me know if I can help :) @Sayanc93 |
|
I am closing this due to long inactivity. Please feel free to reopen if this should be continued to work on. |
This is an attempt to address #7952
and is a work in progress PR (as of now).
Would love some suggestions and guidance on the changes.
Tests and documentation updates are pending on this.
Any help on why I'm not able to runnode test/parallel/test-readline-interface.js?it fails with
Error: Cannot find module 'internal/readline'Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes