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

Feature request: Inform the "line" event on a Readline interface of the type of end-of-line input #7952

Closed
loilo opened this issue Aug 2, 2016 · 14 comments
Labels
feature request Issues that request new features to be added to Node.js. readline Issues and PRs related to the built-in readline module. stale

Comments

@loilo
Copy link

loilo commented Aug 2, 2016

I was using the Readline module to rewrite a file line by line. While doing that I noticed that I had no idea which end-of-line (\r, \n, \r\n) caused the line break so I could not reassemble the file exactly as it was.

Of course I could detect the first EOL and then just use that for everything but in most cases I want to leave the line untouched anyway. Wouldn't it be a good idea to pass the found EOL string as a second argument to the callback of a line event emitted by a readline.Interface?

@addaleax addaleax added readline Issues and PRs related to the built-in readline module. feature request Issues that request new features to be added to Node.js. labels Aug 2, 2016
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 9, 2016

@loilo

detect the first EOL and then just use that for everything

It is not a reliable approach unless you read the whole file first.

  1. A stream chunk could be aborted between \r and \n in \r\n.
  2. You can't be sure if there is a line ending in the end of a file.

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

This is good idea, I think. It would definitely need to be line-by-line because a single doc could contain a mix. The key challenge with implementing this is that the current impl does not record the line-ending currently. It wouldn't be that difficult to add in tho.

Interested in doing a PR? :-)

@loilo
Copy link
Author

loilo commented Aug 9, 2016

I'd love to. I'll give it a try but it's quite possible that I'll just fail to understand the bigger picture of the project. Got a pretty promising looks on the readline.js file though so let's see what comes around, :)

@addaleax
Copy link
Member

addaleax commented Aug 9, 2016

@loilo Cool! If there’s anything you need or you’d like to ask, you can do that here or e.g. in #node-dev on Freenode!

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

+1 to what @addaleax said :-) If you get stuck or need someone to bounce ideas off of, just let one of us know!

@JungMinu
Copy link
Member

JungMinu commented Aug 10, 2016

I'd love to do a PR, please let me know if I can help :)

@addaleax
Copy link
Member

I think @loilo is already working on something? :)

@loilo
Copy link
Author

loilo commented Aug 10, 2016

I started with something but I'd really be completely fine to give that job over to @JungMinu since I realized that I'm actually pretty occupied with more important programming stuff for the next couple of weeks.
So @JungMinu, if you want to give it a shot, go for it. 👍

@JungMinu
Copy link
Member

Yep, I will do a PR soon @loilo @addaleax

@JungMinu JungMinu self-assigned this Aug 11, 2016
@Trott
Copy link
Member

Trott commented Jul 9, 2017

@JungMinu Are you still planning on doing this? If not, should we unassign it so someone else can pick it up?

@gireeshpunathil
Copy link
Member

ping @JungMinu

Sayanc93 added a commit to Sayanc93/node that referenced this issue Oct 27, 2018
pass separator as a parameter to the callback
alongwith line to inform line event about separator
used to break lines
fixes: nodejs#7952 (comment)
@BridgeAR
Copy link
Member

I just planned on implementing this I came to a point where it's difficult to identify the actual end-of-line.

Readline just receives arbitrary chunks and we have to delay emitting the last line in case the chunk contains a carriage return at the end of it. Otherwise it's not possible to guarantee that it's the correct line ending. We also have the crlfDelay option with a default of 100 ms. That means we'll have to trigger a timeout in such cases to emit the line in case no new chunk is incoming during that time. Having such a delay seems pretty bad for most applications, even though it's probably a pretty rare case.
It should also probably only work in case the terminal option is set to false.

I am for these reasons not sure anymore if we should really implement this.
Any opinions?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. readline Issues and PRs related to the built-in readline module. stale
Projects
None yet
Development

No branches or pull requests

8 participants