From 367e076670604d849a381bc004959480ce9c3acd Mon Sep 17 00:00:00 2001 From: Kyrylo Silin Date: Wed, 27 Jun 2012 14:54:07 +0300 Subject: [PATCH] Change behavior of `cd -` command Since banister begged me to do that... completely rewrite `cd -` command (implemetation is much simpler now). This commit brings such changes: * completely rewrite behavior of `cd -` command; * implement ScratchPad aka Pad for unit testing purposes (by banister); * use Pad riches in the unit tests for `cd -` command; * remove verbose and clunky unit tests; This commit brings new meaning to the `cd -` command. The main difference is that the new command saves entire binding stack, not just the last binding. Let me show you an example of the variance between these two implemetations: * Old `cd -` implementation saves *only* last binding. With our next `cd -` invocation our interjacent results are lost: [1] pry(main)> cd 1/2/3/../4 [2] pry(4):3> cd - [3] pry(main)> cd - [4] pry(4):1> nesting Nesting status: -- 0. main (Pry top level) 1. 4 Also, there are a few bugs in old `cd -` command: * you type `cd :foo`, `cd 1/2/3` and `cd -`. The last command relocates you to the scope of `3` (leaves you where you was), when `:foo` is expected; * you type `cd :foo`, `cd 1/2/3/../4`, `cd -`. The last command relocates you to the scope of `3`, when `:foo` is expected. * New and shiny `cd -` is devoid of those shortcomings: [1] pry(main)> cd 1/2/3/../4 [2] pry(4):3> cd - [3] pry(main)> cd - [4] pry(4):3> nesting Nesting status: -- 0. main (Pry top level) 1. 1 2. 2 3. 4 As I said before, this solution is *much* simpler and less error-prone. Signed-off-by: Kyrylo Silin --- lib/pry.rb | 11 +- lib/pry/default_commands/cd.rb | 71 ++--- test/helper.rb | 7 +- test/test_default_commands/test_cd.rb | 357 +++++++------------------- 4 files changed, 128 insertions(+), 318 deletions(-) diff --git a/lib/pry.rb b/lib/pry.rb index 0c1013d2..cc272977 100644 --- a/lib/pry.rb +++ b/lib/pry.rb @@ -103,16 +103,17 @@ class Pry # 3) In a nested session - behave like `cd ..` (pop a binding) DEFAULT_CONTROL_D_HANDLER = proc do |eval_string, _pry_| if !eval_string.empty? - # clear input buffer + # Clear input buffer. eval_string.replace("") elsif _pry_.binding_stack.one? - # ^D at top-level breaks out of a REPL loop + # ^D at top-level breaks out of a REPL loop. _pry_.binding_stack.clear throw(:breakout) else - # otherwise just pops a binding and stores it as old binding - _pry_.command_state["cd"].old_binding = _pry_.binding_stack.pop - _pry_.command_state["cd"].append = true + # Otherwise, saves current binding stack as old stack and pops last + # binding out of binding stack (the old stack still has that binding). + _pry_.command_state["cd"].old_stack = _pry_.binding_stack.dup + _pry_.binding_stack.pop end end diff --git a/lib/pry/default_commands/cd.rb b/lib/pry/default_commands/cd.rb index 730ced77..cbdbbab3 100644 --- a/lib/pry/default_commands/cd.rb +++ b/lib/pry/default_commands/cd.rb @@ -24,48 +24,47 @@ class Pry def process # Extract command arguments. Delete blank arguments like " ", but # don't delete empty strings like "". - path = arg_string.split(/\//).delete_if { |a| a =~ /\A\s+\z/ } - stack = _pry_.binding_stack.dup - - # Save current state values for the sake of restoring them them later - # (for example, when an exception raised). - old_binding = state.old_binding - append = state.append + path = arg_string.split(/\//).delete_if { |a| a =~ /\A\s+\z/ } + stack = _pry_.binding_stack.dup + old_stack = state.old_stack || [] # Special case when we only get a single "/", return to root. if path.empty? - set_old_binding(stack.last, true) if old_binding + state.old_stack = stack.dup unless old_stack.empty? stack = [stack.first] end - path.each do |context| + path.each_with_index do |context, i| begin case context.chomp when "" - set_old_binding(stack.last, true) + state.old_stack = stack.dup stack = [stack.first] when "::" - set_old_binding(stack.last, false) + state.old_stack = stack.dup stack.push(TOPLEVEL_BINDING) when "." next when ".." unless stack.size == 1 - set_old_binding(stack.pop, true) + # Don't rewrite old_stack if we're in complex expression + # (e.g.: `cd 1/2/3/../4). + state.old_stack = stack.dup if path.first == ".." + stack.pop end when "-" - if state.old_binding - toggle_old_binding(stack, old_binding, append) + unless old_stack.empty? + # Interchange current stack and old stack with each other. + stack, state.old_stack = state.old_stack, stack end else - unless path.length > 1 - set_old_binding(stack.last, false) - end + state.old_stack = stack.dup if i == 0 stack.push(Pry.binding_for(stack.last.eval(context))) end rescue RescuableException => e - set_old_binding(old_binding, append) # Restore previous values. + # Restore old stack to its initial values. + state.old_stack = old_stack output.puts "Bad object path: #{arg_string.chomp}. Failed trying to resolve: #{context}" output.puts e.inspect @@ -76,42 +75,6 @@ class Pry _pry_.binding_stack = stack end - private - - # Toggle old binding value by either appending it to the current stack - # (when `append` is `true`) or setting the new one (when `append` is - # `false`). - # - # @param [Array] stack The current stack of bindings. - # @param [Binding] old_binding The old binding. - # @param [Boolean] append The adjunction flag. - # - # @return [Binding] The new old binding. - def toggle_old_binding(stack, old_binding, append) - if append - stack.push(old_binding) - old_binding = stack[-2] - else - old_binding = stack.pop - end - append = !append - - set_old_binding(old_binding, append) - - old_binding - end - - # Set new old binding and adjunction flag. - # - # @param [Binding] binding The old binding. - # @param [Boolean] append The adjunction flag. - # - # @return [void] - def set_old_binding(binding, append) - state.old_binding = binding - state.append = append - end - end end end diff --git a/test/helper.rb b/test/helper.rb index 144e01a3..5201db2c 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -8,6 +8,12 @@ puts "Ruby v#{RUBY_VERSION} (#{defined?(RUBY_ENGINE) ? RUBY_ENGINE : "ruby"}), P require 'bacon' require 'open4' +# A global space for storing temporary state during tests. +ScratchPad = OpenStruct.new +Pad = ScratchPad +def ScratchPad.clear + @table = {} +end # turn warnings off (esp for Pry::Hooks which will generate warnings # in tests) @@ -56,7 +62,6 @@ class TestClassForShowSourceInstanceEval end end - # in case the tests call reset_defaults, ensure we reset them to # amended (test friendly) values class << Pry diff --git a/test/test_default_commands/test_cd.rb b/test/test_default_commands/test_cd.rb index 50a41491..542f65de 100644 --- a/test/test_default_commands/test_cd.rb +++ b/test/test_default_commands/test_cd.rb @@ -1,272 +1,156 @@ require 'helper' describe 'Pry::DefaultCommands::Cd' do + before do + @o = Object.new + + @os1 = "Pad.os1 = _pry_.command_state['cd'].old_stack.dup" + @os2 = "Pad.os2 = _pry_.command_state['cd'].old_stack.dup" + + @bs1 = "Pad.bs1 = _pry_.binding_stack.dup" + @bs2 = "Pad.bs2 = _pry_.binding_stack.dup" + @bs3 = "Pad.bs3 = _pry_.binding_stack.dup" + end + after do - $obj = nil + Pad.clear end describe 'state' do it 'should not to be set up in fresh instance' do - instance = nil - redirect_pry_io(InputTester.new("cd", "exit-all")) do - instance = Pry.new - instance.repl + redirect_pry_io(InputTester.new(@os1, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.should == nil - instance.command_state["cd"].append.should == nil + Pad.os1.should == nil end end - describe 'old binding toggling with `cd -`' do + describe 'old stack toggling with `cd -`' do + describe 'in fresh pry instance' do + it 'should not toggle when there is no old stack' do + redirect_pry_io(InputTester.new("cd -", @bs1, "cd -", @bs2, "exit-all")) do + Pry.start(@o) + end + + Pad.bs1.map { |v| v.eval("self") }.should == [@o] + Pad.bs2.map { |v| v.eval("self") }.should == [@o] + end + end + describe 'when an error was raised' do it 'should ensure cd @ raises SyntaxError' do mock_pry("cd @").should =~ /SyntaxError/ end - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd @", "cd -", "exit-all")) do - instance = Pry.new - instance.repl + it 'should not toggle and should keep correct old stack' do + redirect_pry_io(InputTester.new("cd @", @os1, "cd -", @os2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.should == nil - instance.command_state["cd"].append.should == nil + Pad.os1.should == [] + Pad.os2.should == [] + end - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd @", "exit-all")) do - instance = Pry.new - instance.repl + it 'should not toggle and should keep correct current binding stack' do + redirect_pry_io(InputTester.new("cd @", @bs1, "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == false - - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd @", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg - instance.command_state["cd"].append.should == true + Pad.bs1.map { |v| v.eval("self") }.should == [@o] + Pad.bs2.map { |v| v.eval("self") }.should == [@o] end end describe 'when using simple cd syntax' do - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "exit-all")) do - instance = Pry.new - instance.repl + it 'should toggle' do + redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", @bs1, + "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == false + Pad.bs1.map { |v| v.eval("self") }.should == [@o] + Pad.bs2.map { |v| v.eval("self") }.should == [@o, :mon_dogg] + end + end + + describe "when using complex cd syntax" do + it 'should toggle with a complex path (simple case)' do + redirect_pry_io(InputTester.new("cd 1/2/3", "cd -", @bs1, + "cd -", @bs2, "exit-all")) do + Pry.start(@o) + end + + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 3] end - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "exit-all")) do - instance = Pry.new - instance.repl + it 'should toggle with a complex path (more complex case)' do + redirect_pry_io(InputTester.new("cd 1/2/3", "cd ../4", "cd -", + @bs1, "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg - instance.command_state["cd"].append.should == true - end - - it 'should toggle with multple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == false + Pad.bs1.map { |v| v.eval('self') }.should == [@o, 1, 2, 3] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 4] end end describe 'series of cd calls' do - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == 42 - instance.command_state["cd"].append.should == false - end - - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == true - end - - it 'should toggle with multple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == 42 - instance.command_state["cd"].append.should == false - end - it 'should toggle with fuzzy `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd 42", "cd -", "exit-all")) do - instance = Pry.new - instance.repl + redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd 42", "cd -", + @bs1, "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == 42 - instance.command_state["cd"].append.should == true + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, 42] end end describe 'when using cd ..' do - before do - $obj = Object.new - $obj.instance_variable_set(:@x, 66) - $obj.instance_variable_set(:@y, 79) + it 'should toggle with a simple path' do + redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", @bs1, + "cd -", @bs2, "exit-all")) do + Pry.start(@o) + end + + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg] end - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "exit-all")) do - instance = Pry.new - instance.repl + it 'should toggle with a complex path' do + redirect_pry_io(InputTester.new("cd 1/2/3/../4", "cd -", @bs1, + "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == true - - redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == 66 - instance.command_state["cd"].append.should == true - end - - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == false - - redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == 79 - instance.command_state["cd"].append.should == false - end - - it 'should toggle with multiple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == true - - redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == 66 - instance.command_state["cd"].append.should == true + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 4] end end describe 'when using cd ::' do - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "exit-all")) do - instance = Pry.new - instance.repl + it 'should toggle' do + redirect_pry_io(InputTester.new("cd ::", "cd -", @bs1, + "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == false - end - - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == true - end - - it 'should toggle with multiple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == false + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, TOPLEVEL_BINDING.eval("self")] end end describe 'when using cd /' do - it 'should keep correct old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "exit-all")) do - instance = Pry.new - instance.repl + it 'should toggle' do + redirect_pry_io(InputTester.new("cd /", "cd -", @bs1, "cd :john_dogg", + "cd /", "cd -", @bs2, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == true - end - - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self") - instance.command_state["cd"].append.should == false - end - - it 'should toggle with multiple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :john_dogg - instance.command_state["cd"].append.should == true + Pad.bs1.map { |v| v.eval('self') }.should == [@o] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg] end end @@ -276,59 +160,16 @@ describe 'Pry::DefaultCommands::Cd' do end it 'should keep correct old binding' do - instance = nil redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg", - "cd :kyr_dogg", @control_d, "exit-all")) do - instance = Pry.new - instance.repl + "cd :kyr_dogg", @control_d, @bs1, "cd -", + @bs2, "cd -", @bs3, "exit-all")) do + Pry.start(@o) end - instance.command_state["cd"].old_binding.eval("self").should == :kyr_dogg - instance.command_state["cd"].append.should == true + Pad.bs1.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg] + Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg, :kyr_dogg] + Pad.bs3.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg] end - - it 'should toggle with a single `cd -` call' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg", - "cd :kyr_dogg", @control_d, "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg - instance.command_state["cd"].append.should == false - end - - it 'should toggle with multiple `cd -` calls' do - instance = nil - redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg", - "cd :kyr_dogg", @control_d, "cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.eval("self").should == :kyr_dogg - instance.command_state["cd"].append.should == true - end - end - - it 'should not toggle when there is no old binding' do - instance = nil - redirect_pry_io(InputTester.new("cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.should == nil - instance.command_state["cd"].append.should == nil - - redirect_pry_io(InputTester.new("cd -", "cd -", "exit-all")) do - instance = Pry.new - instance.repl - end - - instance.command_state["cd"].old_binding.should == nil - instance.command_state["cd"].append.should == nil end end