Add history to the coffee interactive interpreter that persists between ... #2886

Merged
merged 8 commits into from Mar 27, 2013

4 participants

@danielgtaylor

...sessions using a ~/.coffee_history file in a similar way to bash. Code based on repl.history and Node pull request 3178 with modifications.

https://github.com/tmpvar/repl.history
https://github.com/joyent/node/pull/3178/files

Modifications:

  • Converted to CoffeeScript
  • Use ~/.coffee_history filename by default
  • Only read up to 10240 bytes of history instead of everything
  • Set default history index to -1 (i.e. pressing up will then show the last saved command first)
  • Make history file optional, disabled with {historyFile: false} in repl.start opts
  • Added unit test to read and process a line from a dummy history file

Tested locally on Mac with Node 0.10 and CoffeeScript 1.6.2 and it is working very nicely for me. There may be path issues between different operating systems, however.

@danielgtaylor danielgtaylor Add history to the coffee interactive interpreter that persists betwe…
…en sessions using a ~/.coffee_history file in a similar way to bash. Code based on repl.history and Node pull request 3178 with modifications.
9bfafb8
@michaelficarra michaelficarra commented on an outdated diff Mar 25, 2013
lib/coffee-script/repl.js
@@ -86,6 +90,53 @@
});
};
+ addHistory = function(repl, filename) {
+ var buffer, e, fd, size, stat;
+ if (filename == null) {
+ filename = path.join(process.env.HOME, '.coffee_history');
+ }
+ try {
+ stat = fs.statSync(filename);
+ size = Math.min(10240, stat.size);
+ fd = fs.openSync(filename, 'r');
+ buffer = new Buffer(size);
+ fs.readSync(fd, buffer, 0, size, stat.size - size);
+ repl.rli.history = buffer.toString().split('\n').reverse();
+ repl.rli.history.shift();
+ console.log(repl.rli.historyIndex);
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

You forgot to rebuild.

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

Very nice additions @danielgtaylor 😄

@vendethiel vendethiel and 1 other commented on an outdated diff Mar 25, 2013
src/repl.coffee
@@ -76,6 +78,41 @@ addMultilineHandler = (repl) ->
rli.prompt true
return
+# Store and load command history from a file
+addHistory = (repl, filename = path.join(process.env.HOME, '.coffee_history')) ->
+ try
+ # Get file info and at most 10KB of command history
+ stat = fs.statSync filename
+ size = Math.min 10240, stat.size
+ # Read last `size` bytes from the file
+ fd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync fd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ repl.rli.history.shift()
+ repl.rli.historyIndex = -1
+ catch e
@vendethiel
Collaborator
vendethiel added a line comment Mar 25, 2013

do we need it ?

@danielgtaylor
danielgtaylor added a line comment Mar 25, 2013

This is a pretty standard feature of almost every shell and even some other interactive interpreters (e.g. ipython does this and is very highly regarded). If we are worried about the extra memory overhead and dumping a new file onto people's machines then maybe this could be disabled by an environment variable or with something like coffee -i --no-history. Thoughts? I'd really love to see this feature get into CoffeeScript.

@vendethiel
Collaborator
vendethiel added a line comment Mar 25, 2013

Sure, why not. I'm only talking about the explicit catch

@danielgtaylor
danielgtaylor added a line comment Mar 25, 2013

Oh, I see. I wasn't aware that CoffeeScript will add a blank catch for you when converting to Javascript. Let me remove it.

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

I've gone ahead and made the history file optional (though this is not exposed to the coffee command as an option, just optional in the repl.start call) and have added a unit test. Thanks for the feedback so far guys!

@michaelficarra michaelficarra and 1 other commented on an outdated diff Mar 25, 2013
src/repl.coffee
+# Store and load command history from a file
+addHistory = (repl, filename) ->
+ try
+ # Get file info and at most 10KB of command history
+ stat = fs.statSync filename
+ size = Math.min 10240, stat.size
+ # Read last `size` bytes from the file
+ fd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync fd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ repl.rli.history.shift()
+ repl.rli.historyIndex = -1
+
+ fd = fs.openSync filename, 'a'
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

I don't like how this fd variable is the same as the one up there in the try, yet used for independent purposes.

@danielgtaylor
danielgtaylor added a line comment Mar 25, 2013

I've gone ahead and updated it to readFd in the latest diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra commented on an outdated diff Mar 25, 2013
src/repl.coffee
+ size = Math.min 10240, stat.size
+ # Read last `size` bytes from the file
+ fd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync fd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ repl.rli.history.shift()
+ repl.rli.historyIndex = -1
+
+ fd = fs.openSync filename, 'a'
+
+ repl.rli.addListener 'line', (code) ->
+ if code and code.length and code isnt '.history'
+ # Save the latest command in the file
+ fs.write fd, code + '\n'
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

String concatenation should always use interpolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra and 1 other commented on an outdated diff Mar 25, 2013
test/repl.coffee
@@ -97,3 +106,6 @@ testRepl "keeps running after runtime error", (input, output) ->
eq 0, output.lastWrite().indexOf 'ReferenceError: b is not defined'
input.emitLine 'a'
eq 'undefined', output.lastWrite()
+
+testRepl 'remove history file', (input, output) ->
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

this should not be a test

@danielgtaylor
danielgtaylor added a line comment Mar 25, 2013

Would you accept something like process.on 'exit' or something like that instead? I'm not sure how you do test teardowns with Node

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danielgtaylor danielgtaylor Use separate variable for fd used to read history file; use string in…
…terpolation to print code lines; do not use unit test to remove temporary file and instead use a process exit event handler
a1ff4ae
@michaelficarra michaelficarra commented on an outdated diff Mar 25, 2013
src/repl.coffee
+ fd = fs.openSync filename, 'a'
+
+ repl.rli.addListener 'line', (code) ->
+ if code and code.length and code isnt '.history'
+ # Save the latest command in the file
+ fs.write fd, "#{code}\n"
+
+ process.on 'exit', ->
+ fs.closeSync fd
+
+ # Add a command to show the history stack
+ repl.commands['.history'] =
+ help: 'Show command history'
+ action: ->
+ out = (repl.rli.history[k] for k in Object.keys(repl.rli.history))
+ repl.outputStream.write out.reverse().join('\n') + '\n'
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

More string concatenation using +.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra commented on an outdated diff Mar 25, 2013
src/repl.coffee
@@ -76,6 +79,39 @@ addMultilineHandler = (repl) ->
rli.prompt true
return
+# Store and load command history from a file
+addHistory = (repl, filename) ->
+ try
+ # Get file info and at most 10KB of command history
+ stat = fs.statSync filename
+ size = Math.min 10240, stat.size
+ # Read last `size` bytes from the file
+ readFd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync readFd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ repl.rli.history.shift()
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 25, 2013

I may be confused here, so correct me if I'm wrong. If you read input starting from an arbitrary position and split by \n, the first element is likely a partial line. If you then reverse it, the last element is likely a partial line. Wouldn't you pop that off instead of shifting it? And wouldn't we need to shift off the empty string that follows the file's trailing newline?

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

Y'all don't think this is way too fancy for the CoffeeScript REPL?

@michaelficarra
Collaborator

Nope, I think it's appropriately fancy.

danielgtaylor added some commits Mar 26, 2013
@danielgtaylor danielgtaylor Use string interpolation for .history command; rename out variable to…
… history an store the history in it as an array in the proper order so that printing it is just a join operation
2d0e45c
@danielgtaylor danielgtaylor Pop off partial line if input history file was truncated; make maximu…
…m input history file size configurable via repl start options
4dbd9dc
@epidemian

Me too. It can be quite useful for REPL-intensive users.

@danielgtaylor

I don't think it's too fancy. Plenty of shells have this feature, and we already have per-session history built-in so this change is really just making it persist between sessions. I really think a lot of people would appreciate this feature.

@danielgtaylor

I believe I've addressed all the issues from the code reviews and we now have several people interested in getting this merged in. Let me know if there is anything else I can do to get this accepted.

@jashkenas jashkenas merged commit 8be65de into jashkenas:master Mar 27, 2013
@michaelficarra michaelficarra referenced this pull request in michaelficarra/CoffeeScriptRedux Mar 29, 2013
Closed

add persistent history to REPL #185

@michaelficarra
Collaborator

This pull request wasn't quite ready. It still has some stylistic issues.

@michaelficarra michaelficarra commented on the diff Mar 29, 2013
src/repl.coffee
+
+ fd = fs.openSync filename, 'a'
+
+ repl.rli.addListener 'line', (code) ->
+ if code and code.length and code isnt '.history'
+ # Save the latest command in the file
+ fs.write fd, "#{code}\n"
+
+ process.on 'exit', ->
+ fs.closeSync fd
+
+ # Add a command to show the history stack
+ repl.commands['.history'] =
+ help: 'Show command history'
+ action: ->
+ history = (repl.rli.history[k] for k in Object.keys(repl.rli.history)).reverse()
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 29, 2013

Mainly this ridiculous monster. Should be

repl.outputStream.write "#{repl.rli.history[..].reverse().join '\n'}\n"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra commented on the diff Mar 29, 2013
src/repl.coffee
@@ -76,6 +80,42 @@ addMultilineHandler = (repl) ->
rli.prompt true
return
+# Store and load command history from a file
+addHistory = (repl, filename, maxSize) ->
+ try
+ # Get file info and at most 10KB of command history
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 29, 2013

Should be maxSize now that you've parameterised that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra commented on the diff Mar 29, 2013
src/repl.coffee
@@ -76,6 +80,42 @@ addMultilineHandler = (repl) ->
rli.prompt true
return
+# Store and load command history from a file
+addHistory = (repl, filename, maxSize) ->
+ try
+ # Get file info and at most 10KB of command history
+ stat = fs.statSync filename
+ size = Math.min maxSize, stat.size
+ # Read last `size` bytes from the file
+ readFd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync readFd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ # If the history file was truncated we should pop off a potential partial line
+ repl.rli.history.pop() if size is maxSize
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 29, 2013

Should be stat.size > maxSize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra commented on the diff Mar 29, 2013
src/repl.coffee
+ # Read last `size` bytes from the file
+ readFd = fs.openSync filename, 'r'
+ buffer = new Buffer(size)
+ fs.readSync readFd, buffer, 0, size, stat.size - size
+ # Set the history on the interpreter
+ repl.rli.history = buffer.toString().split('\n').reverse()
+ # If the history file was truncated we should pop off a potential partial line
+ repl.rli.history.pop() if size is maxSize
+ # Shift off the final blank newline
+ repl.rli.history.shift() if repl.rli.history[0] is ''
+ repl.rli.historyIndex = -1
+
+ fd = fs.openSync filename, 'a'
+
+ repl.rli.addListener 'line', (code) ->
+ if code and code.length and code isnt '.history'
@michaelficarra
Collaborator
michaelficarra added a line comment Mar 29, 2013

To behave like readline's history, you also need to make sure this line is not the same as the last one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelficarra michaelficarra added a commit to michaelficarra/CoffeeScriptRedux that referenced this pull request Mar 29, 2013
@michaelficarra michaelficarra fixes #185: add persistent history to REPL 1daecc1
@danielgtaylor

@michaelficarra I haven't forgotten about your comments, just in the middle of a big move to a new house and without Internet access for a few more days, then it's on my list to get those comments addressed!

@danielgtaylor

@michaelficarra danielgtaylor/coffee-script@feaea49 should fix your issues above. Not sure why it isn't showing here - maybe @jashkenas needs to reopen this pull request, or I can create a new one.

@jashkenas
Owner

It was merged already -- I can't reopen. Feel free to create a new one.

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