Skip to content

Persist REPL history#695

Merged
gkz merged 1 commit into
gkz:masterfrom
raine:repl-history
Dec 17, 2015
Merged

Persist REPL history#695
gkz merged 1 commit into
gkz:masterfrom
raine:repl-history

Conversation

@raine

@raine raine commented Mar 27, 2015

Copy link
Copy Markdown
Contributor

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

gkz commented Mar 28, 2015

Copy link
Copy Markdown
Owner

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

raine commented Mar 28, 2015

Copy link
Copy Markdown
Contributor Author

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

gkz commented Mar 28, 2015

Copy link
Copy Markdown
Owner

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

heavyk commented Mar 29, 2015

Copy link
Copy Markdown

+1 on that idea ...

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

@raine

raine commented Apr 3, 2015

Copy link
Copy Markdown
Contributor Author

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

@gkz

gkz commented Apr 5, 2015

Copy link
Copy Markdown
Owner

500?

@raine

raine commented Apr 11, 2015

Copy link
Copy Markdown
Contributor Author

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 repl-history branch 2 times, most recently from 2463f52 to 6c5bdb9 Compare April 11, 2015 19:45
@phanimahesh

Copy link
Copy Markdown

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

@raine

raine commented Apr 11, 2015

Copy link
Copy Markdown
Contributor Author

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

gkz commented Apr 11, 2015

Copy link
Copy Markdown
Owner

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

blvz commented Apr 11, 2015

Copy link
Copy Markdown
Contributor

lsc --config history=true :)

@raine

raine commented Apr 11, 2015

Copy link
Copy Markdown
Contributor Author

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

@gkz

gkz commented Apr 11, 2015

Copy link
Copy Markdown
Owner

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

@phanimahesh

Copy link
Copy Markdown

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

gkz commented Apr 14, 2015

Copy link
Copy Markdown
Owner

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

@raine

raine commented Apr 15, 2015

Copy link
Copy Markdown
Contributor Author

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

@gkz

gkz commented Apr 15, 2015

Copy link
Copy Markdown
Owner

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

@gkz gkz added the feature label Apr 18, 2015
@raine raine force-pushed the repl-history branch 3 times, most recently from f244167 to b263a87 Compare April 27, 2015 04:42
@raine

raine commented Apr 29, 2015

Copy link
Copy Markdown
Contributor Author

Is there anything I can do to help with this?

@raine

raine commented May 24, 2015

Copy link
Copy Markdown
Contributor Author

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

auvipy commented Dec 1, 2015

Copy link
Copy Markdown

@gkz will this be in?

Comment thread src/command.ls

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.

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
@gkz gkz merged commit bfaea60 into gkz:master Dec 17, 2015
@gkz

gkz commented Dec 17, 2015

Copy link
Copy Markdown
Owner

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants