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

Persist REPL history #695

Merged
merged 1 commit into from Dec 17, 2015
Merged

Persist REPL history #695

merged 1 commit into from Dec 17, 2015

Conversation

@raine
Copy link
Contributor

@raine raine commented Mar 27, 2015

Makes LiveScript REPL history persist across sessions. Writes to $HOME/.lsc_history.

I did not implement any upper limit on the history file size after which the history would be truncated. Will do that if necessary.

New to this project so let me know if the changes don't follow this project's general coding style.

Cheers.

@gkz
Copy link
Owner

@gkz gkz commented Mar 28, 2015

I'm a bit weary of writing a file to a user's home directory by default.
Certainly, there would need to be a limit to how many lines it can hold.

@raine
Copy link
Contributor Author

@raine raine commented Mar 28, 2015

I share that concern as well, as much as I'd like this to be default because it's really useful.

Do you think this should be enabled with an environment variable like LSC_HISTORY=1 or a flag in lsc? The latter would require history users to use an alias with lsc.

@gkz
Copy link
Owner

@gkz gkz commented Mar 28, 2015

Maybe perhaps: if the file exists, use it and write to it, and if it doesn't, don't create it. This way a user would simply have to touch .lsc_history

@heavyk
Copy link

@heavyk heavyk commented Mar 29, 2015

+1 on that idea ...

obviously if the file exceeds 1MB or something, it should be truncated, yeah

@raine
Copy link
Contributor Author

@raine raine commented Apr 3, 2015

What do you think is a good amount of history to have and that shouldn't be exceeded?

@gkz
Copy link
Owner

@gkz gkz commented Apr 5, 2015

500?

@raine raine force-pushed the raine:repl-history branch from 3b29ed7 to 7b8815e Apr 11, 2015
@raine
Copy link
Contributor Author

@raine raine commented Apr 11, 2015

OK, updated to save history only if ~/.lsc_history exists and to retain max 500 lines.

Because of the max line restriction, I changed the implementation to write history to file on exit instead on appending to file descriptor on each line entered. It was easier to do that way.

@raine raine force-pushed the raine:repl-history branch 2 times, most recently from 2463f52 to 6c5bdb9 Apr 11, 2015
@phanimahesh
Copy link

@phanimahesh phanimahesh commented Apr 11, 2015

Most shells by default also add to history files on exit, so that should™ be okay.

@raine
Copy link
Contributor Author

@raine raine commented Apr 11, 2015

Personally I'd opt to write history by default now that there's the max line limit. Having to touch the file manually makes the feature hard to discover, but I'm happy to go with it as it is.

@gkz
Copy link
Owner

@gkz gkz commented Apr 11, 2015

What if I add an option to the command line, --toggle-history or something like that, that will add or remove the file. So instead of touching it, you can just use the option once and then it will start recording history (and use the option again to remove the file). Your code wouldn't need to change, it would still check if it's there before writing.

@blvz
Copy link
Contributor

@blvz blvz commented Apr 11, 2015

lsc --config history=true :)

@raine
Copy link
Contributor Author

@raine raine commented Apr 11, 2015

Sounds reasonable. That or maybe --repl-history and --no-repl-history.

@gkz
Copy link
Owner

@gkz gkz commented Apr 11, 2015

I like --repl-history and --no-repl-history, I think that's very clear

@phanimahesh
Copy link

@phanimahesh phanimahesh commented Apr 12, 2015

What should be the default behavior? I suggest the following:

  1. When history file doesn't exist
    1. Don't write history by default.
    2. Create the file at start when --repl-history is passed, write to it on exit. This allows you to print a warning if file can't be created.
  2. When history file exists
    1. Honor flags. If --no-repl-history is passed, don't read/write history.
    2. If no flag is specified or --repl-history is specified, read history on start and write on exit.
@gkz
Copy link
Owner

@gkz gkz commented Apr 14, 2015

should using --no-repl-history delete the history file, or simply disable the history for that session?

@raine
Copy link
Contributor Author

@raine raine commented Apr 15, 2015

Not sure if disabling history for a particular session is a very likely use case.

@gkz
Copy link
Owner

@gkz gkz commented Apr 15, 2015

Yes, that's why I think it should delete the file. Perhaps a warning and a (Y/n) before it deletes?

@raine raine force-pushed the raine:repl-history branch from 6c5bdb9 to fb758f2 Apr 15, 2015
@gkz gkz added the feature label Apr 18, 2015
@raine raine force-pushed the raine:repl-history branch 3 times, most recently from f244167 to b263a87 Apr 26, 2015
@raine
Copy link
Contributor Author

@raine raine commented Apr 29, 2015

Is there anything I can do to help with this?

@raine
Copy link
Contributor Author

@raine raine commented May 24, 2015

I'd really like to see this in.

(fixed merge conflicts)

repl: write to .lsc_history only if it exists

repl: retain 500 lines of history
@raine raine force-pushed the raine:repl-history branch from b263a87 to b32276a May 24, 2015
@raine raine mentioned this pull request May 24, 2015
@auvipy
Copy link

@auvipy auvipy commented Dec 1, 2015

@gkz will this be in?

@@ -246,6 +250,8 @@ function output-filename filename, json
# - __^C__: Cancel input if any. Quit otherwise.
# - __??__: <https://github.com/joyent/node/blob/master/lib/readline.js>
!function repl
MAX-HISTORY-SIZE = 500

This comment has been minimized.

@vendethiel

vendethiel Dec 1, 2015
Contributor

maybe we want to move that up and use const just to signify it's a constant. LGTM otherwise

gkz added a commit that referenced this pull request Dec 17, 2015
Persist REPL history
@gkz gkz merged commit bfaea60 into gkz:master Dec 17, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gkz
Copy link
Owner

@gkz gkz commented Dec 17, 2015

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants