Handle Concurrent Sessions and Saving Readline::HISTORY #651
Conversation
lib/irb/history.rb
Outdated
| @@ -51,7 +51,7 @@ def save_history | |||
|
|
|||
| if File.exist?(history_file) && | |||
| File.mtime(history_file) != @loaded_history_mtime | |||
| history = history[@loaded_history_lines..-1] if @loaded_history_lines | |||
| @loaded_history_lines&.times { history.delete_at(0) } | |||
There was a problem hiding this comment.
Maybe there's a better way to do this but Readline::HISTORY is pretty limited in how it can be manipulated.
The original issue here was a no implicit conversion of Range into Integer error because Readline::HISTORY doesn't take a range in [].
There was a problem hiding this comment.
Unlike the previous code, Reline::HISTORY will be changed in this operation. Maybe it works, but it's safer and easy to maintain if it doesn't change HISTORY object itself.
There was a problem hiding this comment.
Good point. Updated.
| HISTORY = Array.new | ||
| class TestInputMethodWithRelineHistory < TestInputMethod | ||
| # When IRB.conf[:USE_MULTILINE] is true, IRB::ReidlineInputMethod uses Reline::History | ||
| HISTORY = Reline::History.new(Reline.core.config) |
There was a problem hiding this comment.
Changed from an Array to the history class that is used by default when :USE_MULTILINE is on.
|
|
||
| include IRB::HistorySavingAbility | ||
| end | ||
|
|
||
| class TestInputMethodWithReadlineHistory < TestInputMethod | ||
| # When IRB.conf[:USE_MULTILINE] is false, IRB::ReadlineInputMethod uses Readline::HISTORY | ||
| HISTORY = Readline::HISTORY |
There was a problem hiding this comment.
I created an additional input method test class that uses Readline::HISTORY. At this point, I've only made use of it in the test_history_concurrent_use test method to illustrate the bug. I wasn't sure if we want to test both input methods for all tests.
| io.class::HISTORY.clear | ||
| io.load_history | ||
| io.class::HISTORY.concat(%w"line1 line2") | ||
| io.class::HISTORY << 'line1' | ||
| io.class::HISTORY << 'line2' |
There was a problem hiding this comment.
concat isn't available for Readline::HISTORY. I updated this logic so that this test could use TestInputMethodWithReadlineHistory if we wanted.
| yield IRB.rc_file("_history") | ||
| io.class::HISTORY.replace(history) |
There was a problem hiding this comment.
Readline::HISTORY doesn't respond to replace. Updated so that this works for both input methods. It's also assigned as a constant so it can't just be reassigned or we hit dynamic constant assignment errors.
| end | ||
| io.class::HISTORY.concat(input.split) | ||
| input.split.each { |line| io.class::HISTORY << line } |
There was a problem hiding this comment.
Readline::HISTORY doesn't respond to concat. Updated how this adds the new lines.
lib/irb/history.rb
Outdated
| hist = [] | ||
| from.upto(self.class::HISTORY.size - 1) do |index| | ||
| hist << self.class::HISTORY[index].split("\n").join("\\\n") | ||
| end |
There was a problem hiding this comment.
This code does not look rubyish compared to the original history = history[@loaded_history_lines..-1] if @loaded_history_lines that does not work.
How about changing history to an array first? then it will work fine for Readline::HISTORY too.
def save_history
history = self.class::HISTORY.to_a
test/irb/test_history.rb
Outdated
| class TestInputMethodWithHistory < TestInputMethod | ||
| HISTORY = Array.new | ||
| class TestInputMethodWithRelineHistory < TestInputMethod | ||
| # When IRB.conf[:USE_MULTILINE] is true, IRB::ReidlineInputMethod uses Reline::History |
There was a problem hiding this comment.
IRB::ReidlineInputMethod → IRB::RelineInputMethod
IRB::ReidlineInputMethod is not used anymore.
Other part of the change in test_history.rb looks great 👍
There was a problem hiding this comment.
Thanks. Updated.
There was a problem hiding this comment.
Updated. Locally I'm getting a lot of test errors from TestIRB::DebugCommandTest, probably related to the changes from master that were merged into this branch. I get the errors in master as well. I haven't dug into why. The CI testing appears to be running successfully.
test/irb/test_history.rb
Outdated
| class TestInputMethodWithHistory < TestInputMethod | ||
| HISTORY = Array.new | ||
| class TestInputMethodWithRelineHistory < TestInputMethod | ||
| # When IRB.conf[:USE_MULTILINE] is true, IRB::ReidlineInputMethod uses Reline::History |
There was a problem hiding this comment.
Thanks. Updated.
lib/irb/history.rb
Outdated
| hist = [] | ||
| from.upto(self.class::HISTORY.size - 1) do |index| | ||
| hist << self.class::HISTORY[index].split("\n").join("\\\n") | ||
| end |
(ruby/irb#651) * handle concurrent sessions and saving Readline::HISTORY, fixes ruby/irb#510 * separate tests * don't mutate the HISTORY object on the class * avoid repeated .to_i calls * remove intermediary history array * work with array, fix test comment --------- ruby/irb@1681ada328 Co-authored-by: Stan Lo <stan001212@gmail.com>
Fixes #510.