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

repl: Reverses order of .node_repl_history by zeusdeux · Pull Request #4313 · nodejs/node · GitHub

/ node Public

repl: Reverses order of .node_repl_history - #4313

Closed
zeusdeux wants to merge 1 commit into
nodejs:masterfrom
zeusdeux:issue-3928
ConversationCommits (1)ChecksFiles changed
Closed

repl: Reverses order of .node_repl_history#4313
zeusdeux wants to merge 1 commit into
nodejs:masterfrom
zeusdeux:issue-3928

Conversation

zeusdeux commented Dec 16, 2015

Copy link
Copy Markdown
Contributor

Rationale -> #3928.

A few tests are still broken for this and I am kinda confused as to why. Could use some help to ready this PR up.

Thanks!

zeusdeux mentioned this pull request Dec 16, 2015
Closed
mscdex added the repl Issues and PRs related to the REPL subsystem. label Dec 16, 2015
mscdex reviewed Dec 16, 2015
View reviewed changes
Comment thread lib/internal/repl.js

mscdex Dec 16, 2015

Copy link
Copy Markdown
Contributor

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'm not sure how large history files can get, but maybe it'd be more efficient to limit the input to the last repl.historySize lines first and then split on that? For example:

var reLastLines = new RegExp('(?:.*[\r\n]+){0,' + repl.historySize + '}$');
repl.history = reLastLines.exec(data)[0].match(/[^\r\n]+/g) || [];

Fishrock123 Dec 16, 2015

Copy link
Copy Markdown
Contributor

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 doubt it would be an issue large enough to necessitate that unless some manually edited in a way-too-big history file.

Fishrock123 Dec 16, 2015

Copy link
Copy Markdown
Contributor

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 think this code may not be correct though, I think it should probably keep the original logic, and then call .reverse()?

zeusdeux Dec 17, 2015

Copy link
Copy Markdown
Contributor 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

@Fishrock123 Since the history file is going to be arranged from least recent to most (most being at the bottom), doing data.split(/[\n\r]+/, repl.historySize) will end up losing the latest history elements if data.length > repl.historySize. The latest history elements will end up getting dropped/truncated as they are at a higher index in data array than the less recent ones.

Fishrock123 Dec 17, 2015

Copy link
Copy Markdown
Contributor

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

Oooh good catch.

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 16, 2015

Fishrock123 commented Dec 16, 2015

Copy link
Copy Markdown
Contributor

(Actually, on second thought, this is semver-major anyways)

Fishrock123 reviewed Dec 16, 2015
View reviewed changes
Comment thread test/parallel/test-repl-persistent-history.js

Fishrock123 Dec 16, 2015

Copy link
Copy Markdown
Contributor

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 these are being reversed, something is probably not correct. :)

Fishrock123 commented Dec 16, 2015

Copy link
Copy Markdown
Contributor

@zeusdeux Instead of changing readline, I suggest changing how it is written out to file: https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L173-L182

rvagg commented Jan 4, 2016

Copy link
Copy Markdown
Member

Welcome @zeusdeux, I believe this would be your first commit to core if it gets in, thanks for contributing! Even if this doesn't get in, we appreciate the time you've spent discussing and working on this with us.

I'm labelling this one ctc-agenda as it's not clear to me that the pain of switching even in semver-minor is worth is and the rationale seems to be "most other apps that save history do it this way" which doesn't sound very compelling because there's no reason to be compatible with them as far as I can tell.

zeusdeux commented Jan 5, 2016

Copy link
Copy Markdown
Contributor Author

@rvagg I do have a commit in actually. Got one in quite recently with help from @Fishrock123. I love this community! :)

As for this PR, should I continue working on this then or wait till I hear more?

rvagg commented Jan 6, 2016

Copy link
Copy Markdown
Member

@zeusdeux we'll cover this in our CTC meeting tomorrow, no promises on actually getting anywhere with it. Will let you know.

rvagg mentioned this pull request Jan 6, 2016
Closed

mscdex commented Jan 6, 2016

Copy link
Copy Markdown
Contributor

This is kind of a general idea: if we want to start making changes like this, perhaps we should consider versioning history files (e.g. add some version identifier at the top of the file)? That way we can more easily know how to parse a history file (in the future)?

jasnell commented Jan 6, 2016

Copy link
Copy Markdown
Member

This was discussed on the CTC call today. Overall consensus is that there is currently no pressing reason to adopt this change currently and we'd rather keep the history ordering consistent with what's currently in v4 and v5. Unless there is a compelling new reason to make this change, we'd prefer to keep things as they are currently.

zeusdeux commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell fair enough. Should I go ahead and just close this then?

jasnell commented Jan 7, 2016

Copy link
Copy Markdown
Member

If that's what you wish. If you close but not delete the branch, we can at
least retain the changes so the wish is not lost should we decide later to
go ahead with the modification.
On Jan 7, 2016 5:46 AM, "Mudit Ameta" notifications@github.com wrote:

@jasnell https://github.com/jasnell fair enough. Should I go ahead and
just close this then?

—
Reply to this email directly or view it on GitHub
#4313 (comment).

zeusdeux commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

I don't mind leaving it open but I fear that might just add unnecessary cognitive load for anyone going through the PRs. On the flipside, the issue (#3928) that prompted this PR is still open so closing this might make for a weird history later on. So, yeah, I am not sure.

As for the branch, sure, we can keep it around.

bnoordhuis commented Jan 7, 2016

Copy link
Copy Markdown
Member

Let's close it for now. Like you say, it unclutters the open PRs view.

bnoordhuis closed this Jan 7, 2016
lance mentioned this pull request May 26, 2016
Closed
4 tasks
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

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL