Merge pull request 391 from envygeeks

Tidies up the code surrounding platform checks, and invoking the editor.

Fixes #391
Fixes #386

lib/pry.rb

@@ -157,8 +157,9 @@ require "stringio"
 require "coderay"
 require "optparse"
 require "slop"
+require "rbconfig"
 
-if RUBY_PLATFORM =~ /jruby/
+if Pry::Helpers::BaseHelpers.jruby?
   begin
     require 'ffi'
   rescue LoadError
@@ -166,7 +167,7 @@ if RUBY_PLATFORM =~ /jruby/
   end
 end
 
-if RUBY_PLATFORM =~ /mswin/ || RUBY_PLATFORM =~ /mingw/
+if Pry::Helpers::BaseHelpers.windows?
   begin
     require 'win32console'
   rescue LoadError

lib/pry/helpers/base_helpers.rb

@@ -91,14 +91,19 @@ class Pry
         27
       end
 
+      # have fun on the Windows platform.
+      def windows?
+        RbConfig::CONFIG['host_os'] =~ /mswin|mingw/
+      end
+
       # are we on Jruby platform?
       def jruby?
-        defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /jruby/
+        RbConfig::CONFIG['ruby_install_name'] == 'jruby'
       end
 
       # are we on rbx platform?
       def rbx?
-       defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /rbx/
+        RbConfig::CONFIG['ruby_install_name'] == 'rbx'
       end
 
       # a simple pager for systems without `less`. A la windows.

lib/pry/helpers/command_helpers.rb

@@ -160,6 +160,7 @@ class Pry
       end
 
       def invoke_editor(file, line)
+        raise CommandError, "Please set Pry.config.editor or export $EDITOR" unless Pry.config.editor
         if Pry.config.editor.respond_to?(:call)
           editor_invocation = Pry.config.editor.call(file, line)
         else
@@ -179,14 +180,16 @@ class Pry
           # Note we dont want to use Pry.config.system here as that
           # may be invoked non-interactively (i.e via Open4), whereas we want to
           # ensure the editor is always interactive
-          system(editor_invocation)
+          system(editor_invocation) or raise CommandError, "`#{editor_invocation}` gave exit status: #{$?.exitstatus}"
         end
       end
 
       # Return the syntax for a given editor for starting the editor
       # and moving to a particular line within that file
       def start_line_syntax_for_editor(file_name, line_number)
-        file_name = file_name.gsub(/\//, '\\') if RUBY_PLATFORM =~ /mswin|mingw/
+        if windows?
+          file_name = file_name.gsub(/\//, '\\')
+        end
 
         # special case for 1st line
         return file_name if line_number <= 1
@@ -201,7 +204,7 @@ class Pry
         when /^jedit/
           "#{file_name} +line:#{line_number}"
         else
-          if RUBY_PLATFORM =~ /mswin|mingw/
+          if windows?
             "#{file_name}"
           else
             "+#{line_number} #{file_name}"

lib/pry/pry_class.rb

@@ -192,10 +192,15 @@ class Pry
   end
 
   def self.default_editor_for_platform
-    if RUBY_PLATFORM =~ /mswin|mingw/
-      ENV['VISUAL'] || ENV['EDITOR'] || "notepad"
+    return ENV['VISUAL'] if ENV['VISUAL'] and not ENV['VISUAL'].empty?
+    return ENV['EDITOR'] if ENV['EDITOR'] and not ENV['EDITOR'].empty?
+
+    if Helpers::BaseHelpers.windows?
+      'notepad'
     else
-      ENV['VISUAL'] || ENV['EDITOR'] || "nano"
+      %w(editor nano vi).detect do |editor|
+        system("which #{editor} > /dev/null 2>&1")
+      end
     end
   end
 
@@ -210,12 +215,12 @@ class Pry
     config.exception_whitelist = DEFAULT_EXCEPTION_WHITELIST
     config.hooks = DEFAULT_HOOKS
     config.input_stack = []
-    config.color = Pry::Helpers::BaseHelpers.use_ansi_codes?
+    config.color = Helpers::BaseHelpers.use_ansi_codes?
     config.pager = true
     config.system = DEFAULT_SYSTEM
     config.editor = default_editor_for_platform
     config.should_load_rc = true
-    config.should_trap_interrupts = defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /jruby/
+    config.should_trap_interrupts = Helpers::BaseHelpers.jruby?
     config.disable_auto_reload = false
     config.command_prefix = ""
     config.auto_indent = true

lib/pry/wrapped_module.rb

@@ -54,7 +54,7 @@ class Pry
     def singleton_instance
       raise ArgumentError, "tried to get instance of non singleton class" unless singleton_class?
 
-      if defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /jruby/
+      if Helpers::BaseHelpers.jruby?
         wrapped.to_java.attached
       else
         @singleton_instance ||= ObjectSpace.each_object(wrapped).detect{ |x| (class << x; self; end) == wrapped }

test/helper.rb

@@ -49,16 +49,6 @@ class MockPryException
   end
 end
 
-# are we on Jruby platform?
-def jruby?
-  defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /jruby/
-end
-
-# are we on rbx platform?
-def rbx?
-  defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /rbx/
-end
-
 Pry.reset_defaults
 
 # this is to test exception code (cat --ex)
@@ -183,7 +173,7 @@ end
 
 # to help with tracking down bugs that cause an infinite loop in the test suite
 if ENV["SET_TRACE_FUNC"]
-  require 'set_trace' if defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /rbx/ # gem install 'rbx-tracer'
+  require 'set_trace' if Pry::Helpers::BaseHelpers.rbx?
   set_trace_func proc { |event, file, line, id, binding, classname|
      STDERR.printf "%8s %s:%-2d %10s %8s\n", event, file, line, id, classname
   }

test/test_completion.rb

@@ -16,7 +16,7 @@ describe Pry::InputCompleter do
   end
 
   # another jruby hack :((
-  if !jruby?
+  if !Pry::Helpers::BaseHelpers.jruby?
     it "should not crash if there's a Module that has a symbolic name." do
       completer = Pry::InputCompleter.build_completion_proc(Pry.binding_for(Object.new))
       lambda{ completer.call "a.to_s." }.should.not.raise Exception

test/test_default_commands/test_ls.rb

@@ -92,7 +92,7 @@ describe "ls" do
   describe "when no arguments given" do
     describe "when at the top-level" do
       # rubinius has a bug that means local_variables of "main" aren't reported inside eval()
-      unless defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /rbx/
+      unless Pry::Helpers::BaseHelpers.rbx?
         it "should show local variables" do
           mock_pry("ls").should =~ /_pry_/
           mock_pry("arbitrar = 1", "ls").should =~ /arbitrar/

test/test_default_commands/test_shell.rb

@@ -47,7 +47,7 @@ describe "Pry::DefaultCommands::Shell" do
     # this doesnt work so well on rbx due to differences in backtrace
     # so we currently skip rbx until we figure out a workaround
     describe "with --ex" do
-      if !rbx?
+      if !Pry::Helpers::BaseHelpers.rbx?
         it 'cat --ex should correctly display code that generated exception even if raised in repl' do
           mock_pry("this raises error", "cat --ex").should =~ /\d+:(\s*) this raises error/
         end

test/test_method.rb

@@ -98,7 +98,7 @@ describe Pry::Method do
     end
 
     # Our source_location trick doesn't work, due to https://github.com/rubinius/rubinius/issues/953
-    unless defined?(RUBY_ENGINE) && RUBY_ENGINE =~ /rbx/
+    unless Pry::Helpers::BaseHelpers.rbx?
       it 'should find the super method correctly' do
         a = Class.new{ def gag; binding; end; def self.line; __LINE__; end }
         b = Class.new(a){ def gag; super; end }

test/test_syntax_checking.rb

@@ -26,7 +26,7 @@ describe Pry do
     ["1 1"],
     ["puts :"],
     # in this case the syntax error is "expecting ')'".
-    ((defined? RUBY_ENGINE && RUBY_ENGINE == "rbx") ? nil : ["def", "method(1"])
+    (Pry::Helpers::BaseHelpers.rbx? ? nil : ["def", "method(1"])
   ].compact.each do |foo|
     it "should raise an error on invalid syntax like #{foo.inspect}" do
       output = StringIO.new