FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

readline: inform line event about separator used by Sayanc93 · Pull Request #23929 · nodejs/node · GitHub

/ node Public

readline: inform line event about separator used - #23929

Closed
Sayanc93 wants to merge 1 commit into
nodejs:masterfrom
Sayanc93:inform-line-event-on-readline-about-endline-input
ConversationCommits (1)ChecksFiles changed
Closed

readline: inform line event about separator used#23929
Sayanc93 wants to merge 1 commit into
nodejs:masterfrom
Sayanc93:inform-line-event-on-readline-about-endline-input

Conversation

Sayanc93 commented Oct 27, 2018 •
edited
Loading

Copy link
Copy Markdown

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 run node test/parallel/test-readline-interface.js?
it fails with Error: Cannot find module 'internal/readline'

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Oct 27, 2018

devsnek commented Oct 27, 2018

Copy link
Copy Markdown
Member

to run the test use $ tools/test.py -J test/parallel/test-readline-interface.js.

readline: inform line event about separator used
687cdf5 
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 October 27, 2018 23:53
vsemozhetbyt mentioned this pull request Oct 28, 2018
Closed
4 tasks

Trott commented Nov 4, 2018

Copy link
Copy Markdown
Member

@nodejs/repl This could use some reviews.

BridgeAR reviewed Nov 4, 2018
View reviewed changes
Comment thread lib/readline.js


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

BridgeAR Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Sayanc93 Nov 6, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Comment thread lib/readline.js
// \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)]$/;

BridgeAR Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Sayanc93 Nov 6, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thanks for the insight, I'll check and update

Comment thread lib/readline.js


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

BridgeAR Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Sayanc93 Nov 6, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/readline.js
this._onLine(lineWithoutSeparator, separator);
} else {
this._onLine(_line, '\n');
}

BridgeAR Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

BridgeAR commented Nov 4, 2018

Copy link
Copy Markdown
Member

It would be great to add a test with a file that contains mixed line endings.

Sayanc93 commented Nov 6, 2018

Copy link
Copy Markdown
Author

@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 PRs that contain new features and should be released in the next minor version. label Nov 6, 2018

Trott commented Nov 21, 2018

Copy link
Copy Markdown
Member

Is this still being worked on?

antsmartian commented Jan 26, 2019

Copy link
Copy Markdown
Contributor

Ping @Sayanc93

zero1five commented May 25, 2019

Copy link
Copy Markdown

That seems pretty good, it please let me know if I can help :) @Sayanc93

BridgeAR commented Dec 14, 2019

Copy link
Copy Markdown
Member

I am closing this due to long inactivity. Please feel free to reopen if this should be continued to work on.

BridgeAR closed this Dec 14, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments

Assignees

No one assigned

Labels

readline Issues and PRs related to the built-in readline module. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL