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

repl: do not save history for non-terminal repl #1575

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 1, 2015

When running in non-TTY mode - the repl.history is undefined and
is not actually populated. Saving it will result in a crashes of
subsequent repl runs.

Fix: #1574

When running in non-TTY mode - the `repl.history` is `undefined` and
is not actually populated. Saving it will result in a crashes of
subsequent repl runs.

Fix: nodejs#1574
@indutny
Copy link
Member Author

indutny commented May 1, 2015

R=@chrisdickinson

@indutny indutny mentioned this pull request May 1, 2015
@brendanashworth brendanashworth added the repl Issues and PRs related to the REPL subsystem. label May 1, 2015
@Fishrock123 Fishrock123 added this to the 2.0.0 milestone May 1, 2015
@chrisdickinson
Copy link
Contributor

LGTM, sorry this slipped through.

@indutny
Copy link
Member Author

indutny commented May 1, 2015

@chrisdickinson I had some time to think about it... What is your opinion on ignore JSON.parse errors and emptying the history? I think it is better than crashing the repl, do you agree?

@chrisdickinson
Copy link
Contributor

I think my preference would be to note that we couldn't load history and that support will be disabled until the file is fixed (or removed.)

On May 1, 2015, at 10:54 AM, Fedor Indutny notifications@github.com wrote:

@chrisdickinson I had some time to think about it... What is your opinion on ignore JSON.parse errors and emptying the history? I think it is better than crashing the repl, do you agree?


Reply to this email directly or view it on GitHub.

@indutny
Copy link
Member Author

indutny commented May 1, 2015

Why? Who cares about preserving the history?

@chrisdickinson
Copy link
Contributor

My reasoning is:

  1. it could be pointed at an unintended file, or,
  2. it could somehow have been caught in a bad state (reading a half-written file, perhaps!), or,
  3. it might be the result of a bug in io.js in which case having the output of the file would be useful.

In any case I'd prefer to let the user be the one to blow away the file on error, in case they're relying on the persistent history to "save" their work.

@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label May 2, 2015
indutny added a commit that referenced this pull request May 2, 2015
When running in non-TTY mode - the `repl.history` is `undefined` and
is not actually populated. Saving it will result in a crashes of
subsequent repl runs.

Fix: #1574
PR-URL: #1575
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@indutny
Copy link
Member Author

indutny commented May 2, 2015

Landed in ea5195c, thank you!

@indutny indutny closed this May 2, 2015
@indutny indutny deleted the fix/gh-1574 branch May 2, 2015 01:13
@rvagg rvagg mentioned this pull request May 2, 2015
silverwind added a commit to silverwind/node that referenced this pull request May 2, 2015
Issue nodejs#1575 did introduce a check for options.terminal but this variable
wasn't able to get truthy, which in turn broke persistent history
completely. This changes the variable to get truthy on true terminals.

Additionally, the docs and the code did differ on which environment
variable was used for history. This changes the code to use
NODE_REPL_HISTORY_FILE.
silverwind added a commit that referenced this pull request May 2, 2015
Issue #1575 did introduce a check for options.terminal but this variable
wasn't able to get truthy, which in turn broke persistent history
completely. This changes the variable to get truthy on true terminals.

Additionally, the docs and the code did differ on which environment
variable was used for history. This changes the code to use
NODE_REPL_HISTORY_FILE.

PR-URL: #1593
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL history issue
4 participants