Refactor and test the "edit" command.

lib/pry/default_commands/introspection.rb

@@ -110,26 +110,53 @@ class Pry
 
       command "edit", "Invoke the default editor on a file. Type `edit --help` for more info" do |*args|
         opts = Slop.parse!(args) do |opt|
-          opt.banner "Usage: edit [OPTIONS] [FILE]\n" \
+          opt.banner "Usage: edit [--no-reload|--reload] [--line LINE] [--temp|--ex|FILE[:LINE]]\n" \
-                      "Edit the method FILE in an editor.\nWhen no file given, opens editor with contents of input buffer or previous input and evals after closing." \
+                     "Open a text editor. When no FILE is given, edits the pry input buffer.\n" \
-                      "\nEnsure #{text.bold("Pry.config.editor")} is set to your editor of choice.\n" \
+                     "Ensure #{text.bold("Pry.config.editor")} is set to your editor of choice.\n" \
                      "e.g: edit sample.rb"
 
-          opt.on :r, "reload", "Eval file content after editing (evals at top level)"
+          opt.on :e, :ex, "Open the file that raised the most recent exception (_ex_.file)"
-          opt.on :n, "no-reload", "Do not automatically reload the file after editing (only applies to --ex and -t)."
+          opt.on :t, :temp, "Open an empty temporary file"
-          opt.on :ex, "Open an editor at the line and file that generated the most recent Exception, reloads file after editing."
+          opt.on :l, :line, "Jump to this line in the opened file", true, :as => Integer
-          opt.on :t, "temp", "Open a temporary file in an editor with contents of input buffer and eval it in current context after closing (does not default to previous input)"
+          opt.on :n, :"no-reload", "Don't automatically reload the edited code"
-          opt.on :p, "play", "Use the pry `play` command to eval the file content after editing."
+          opt.on :r, :reload, "Reload the edited code immediately (default for ruby files)"
-          opt.on :l, "line", "Specify line number to jump to in file", true, :as => Integer
           opt.on :h, :help, "This message." do
             output.puts opt
           end
         end
         next if opts.h?
 
-        should_reload_at_top_level = opts[:r]
+        if [opts.ex? || nil, opts.t? || nil, !args.empty? || nil].compact.size > 1
-        should_reload_locally      = false
+          next output.puts "Only one of --ex, --temp, and FILE may be specified"
+        end
 
+        # edit of local code, eval'd within pry.
+        if !opts.ex? && args.empty?
+
+          content = if !eval_string.empty?
+                      eval_string
+                    elsif !opts.t?
+                      _pry_.input_array.reverse_each.find{ |x| x && x.strip != "" } # No present? in 1.8
+                    end || ""
+
+          file_name = temp_file do |f|
+            f.puts(content)
+          end
+          line = content.lines.count
+
+          invoke_editor(file_name, line)
+
+          if !opts.n?
+            silence_warnings do
+              eval_string.replace(File.read(file_name))
+            end
+          end
+
+          # don't leak temporary files
+          File.unlink(file_name)
+
+        # edit of remote code, eval'd at top-level
+        else
           if opts.ex?
             next output.puts "No Exception found." if _pry_.last_exception.nil?
 
@@ -143,44 +170,25 @@ class Pry
             next output.puts "Exception has no associated file." if file_name.nil?
             next output.puts "Cannot edit exceptions raised in REPL." if Pry.eval_path == file_name
 
-          should_reload_at_top_level = opts[:n] ? false : true
-
-        elsif opts.t? || args.empty?
-          file_name = temp_file do |f|
-            if !eval_string.empty?
-              f.puts eval_string
-            elsif !opts.t? && !_pry_.input_array.empty?
-              f.puts _pry_.input_array[-1]
-            end
-          end
-          line = eval_string.lines.count + 1
-          should_reload_locally = opts[:n] ? false : true
           else
             # break up into file:line
-          /(:(\d+))?$/ =~ File.expand_path(args.first)
+            file_name = File.expand_path(args.first)
 
-          # $` is pre-match
+            line = file_name.sub!(/:(\d+)$/, "") ? $1.to_i : 1
-          file_name, line = [$`, $2]
-          line = line ? line.to_i : opts[:l].to_i
           end
 
+          line = opts[:l].to_i if opts.l?
+
           invoke_editor(file_name, line)
           set_file_and_dir_locals(file_name)
 
-        if opts[:p]
+          if opts.r? || ((opts.ex? || file_name.end_with?(".rb")) && !opts.n?)
-          silence_warnings do
-            _pry_.input = StringIO.new(File.readlines(file_name).join)
-          end
-        elsif should_reload_locally
-          silence_warnings do
-            eval_string.replace(File.read(file_name))
-          end
-        elsif should_reload_at_top_level
             silence_warnings do
               TOPLEVEL_BINDING.eval(File.read(file_name), file_name)
             end
           end
         end
+      end
 
       command "edit-method", "Edit a method. Type `edit-method --help` for more info." do |*args|
         target = target()

test/test_default_commands/test_introspection.rb

@@ -1,6 +1,164 @@
 require 'helper'
 
 describe "Pry::DefaultCommands::Introspection" do
+
+  describe "edit" do
+    before do
+      @old_editor = Pry.config.editor
+      @file = nil; @line = nil; @contents = nil
+      Pry.config.editor = lambda do |file, line|
+        @file = file; @line = line; @contents = File.read(@file)
+        ":" # The : command does nothing.
+      end
+    end
+    after do
+      Pry.config.editor = @old_editor
+    end
+
+    describe "with FILE" do
+      it "should invoke Pry.config.editor with absolutified filenames" do
+        mock_pry("edit foo.rb")
+        @file.should == File.expand_path("foo.rb")
+        mock_pry("edit /tmp/bar.rb")
+        @file.should == "/tmp/bar.rb"
+      end
+
+      it "should guess the line number from a colon" do
+        mock_pry("edit /tmp/foo.rb:10")
+        @line.should == 10
+      end
+
+      it "should use the line number from -l" do
+        mock_pry("edit -l 10 /tmp/foo.rb")
+        @line.should == 10
+      end
+
+      it "should not delete the file!" do
+        mock_pry("edit Rakefile")
+        File.exist?(@file).should == true
+      end
+
+      describe do
+        before do
+          @rand = rand
+          Pry.config.editor = lambda { |file, line|
+            File.open(file, 'w') { |f| f << "$rand = #{@rand.inspect}" }
+            ":"
+          }
+        end
+
+        it "should reload the file if it is a ruby file" do
+          path = Tempfile.new(["tmp", ".rb"]).path
+
+          mock_pry("edit #{path}", "$rand").should =~ /#{@rand}/
+
+          File.unlink(path)
+        end
+
+        it "should not reload the file if it is not a ruby file" do
+          path = Tempfile.new(["tmp", ".py"]).path
+
+          mock_pry("edit #{path}", "$rand").should.not =~ /#{@rand}/
+
+          File.unlink(path)
+        end
+
+        it "should not reload a ruby file if -n is given" do
+          path = Tempfile.new(["tmp", ".rb"]).path
+
+          mock_pry("edit -n #{path}", "$rand").should.not =~ /#{@rand}/
+
+          File.unlink(path)
+        end
+
+        it "should reload a non-ruby file if -r is given" do
+          path = Tempfile.new(["tmp", ".pryrc"]).path
+
+          mock_pry("edit -r #{path}", "$rand").should =~ /#{@rand}/
+
+          File.unlink(path)
+        end
+      end
+    end
+
+    describe "with --ex" do
+      before do
+        @path = Tempfile.new(["tmp", ".rb"]).path
+        File.open(@path, 'w'){ |f| f << "1\n2\nraise RuntimeError" }
+      end
+      after do
+        File.unlink(@path)
+        File.unlink("#{@path}c") if File.exists?("#{@path}c") #rbx
+      end
+      it "should open the correct file" do
+        mock_pry("require #{@path.inspect}", "edit --ex")
+
+        @file.should == @path
+        @line.should == 3
+      end
+
+      it "should reload the file" do
+        Pry.config.editor = lambda {|file, line|
+          File.open(file, 'w'){|f| f << "FOO = 'BAR'" }
+          ":"
+        }
+
+        mock_pry("require #{@path.inspect}", "edit --ex", "FOO").should =~ /BAR/
+      end
+
+      it "should not reload the file if -n is passed" do
+        Pry.config.editor = lambda {|file, line|
+          File.open(file, 'w'){|f| f << "FOO2 = 'BAR'" }
+          ":"
+        }
+
+        mock_pry("require #{@path.inspect}", "edit -n --ex", "FOO2").should.not =~ /BAR/
+      end
+    end
+
+    describe "without FILE" do
+      it "should edit the current expression if it's incomplete" do
+        mock_pry("def a", "edit")
+        @contents.should == "def a\n"
+      end
+
+      it "should edit the previous expression if the current is empty" do
+        mock_pry("def a; 2; end", "edit")
+        @contents.should == "def a; 2; end\n"
+      end
+
+      it "should use a blank file if -t is specified" do
+        mock_pry("def a; 5; end", "edit -t")
+        @contents.should == "\n"
+      end
+
+      it "should position the cursor at the end of the expression" do
+        mock_pry("def a; 2;"," end", "edit")
+        @line.should == 2
+      end
+
+      it "should delete the temporary file" do
+        mock_pry("edit")
+        File.exist?(@file).should == false
+      end
+
+      it "should evaluate the expression" do
+        Pry.config.editor = lambda {|file, line|
+          File.open(file, 'w'){|f| f << "'FOO'\n" }
+          ":"
+        }
+        mock_pry("edit").should =~ /FOO/
+      end
+      it "should not evaluate the expression with -n" do
+        Pry.config.editor = lambda {|file, line|
+          File.open(file, 'w'){|f| f << "'FOO'\n" }
+          ":"
+        }
+        mock_pry("edit -n").should.not =~ /FOO/
+      end
+    end
+  end
+
   describe "show-method" do
     it 'should output a method\'s source' do
       str_output = StringIO.new