friendlier output for cycles in interpolation, better exception handling

lib/simplygenius/atmos/cli.rb

@@ -112,6 +112,19 @@ def parse(arguments)
         end
       end
 
+      # Hook into clamp lifecycle to globally handle errors
+      class << self
+        def run(*args, **opts, &blk)
+          begin
+            super
+          rescue Exception => e
+            logger.log_exception(e, "Unhandled exception", level: :debug)
+            logger.error(e.message)
+            exit!
+          end
+        end
+      end
+
     end
 
   end

lib/simplygenius/atmos/exceptions.rb

@@ -1,11 +1,15 @@
 require_relative '../atmos'
+require 'clamp'
 
 module SimplyGenius
   module Atmos
 
     module Exceptions
       class UsageError < Clamp::UsageError
 
+      end
+      class ConfigInterpolationError < ::ScriptError
+
       end
     end
 

lib/simplygenius/atmos/settings_hash.rb

@@ -1,4 +1,5 @@
 require 'hashie'
+require_relative "../atmos/exceptions"
 
 module SimplyGenius
   module Atmos
@@ -7,6 +8,7 @@ class SettingsHash <  Hashie::Mash
       include GemLogger::LoggerSupport
       include Hashie::Extensions::DeepMerge
       include Hashie::Extensions::DeepFetch
+      include SimplyGenius::Atmos::Exceptions
       disable_warnings
 
       PATH_PATTERN = /[\.\[\]]/
@@ -16,20 +18,33 @@ class SettingsHash <  Hashie::Mash
 
       alias orig_reader []
 
-      def expanding_reader(name)
-        value = orig_reader(name)
-
-        if value.nil? && root && enable_expansion
+      def expand_results(name, &blk)
+        # NOTE: looking up globally then locally in order to preserve compatibility
+        # would be better to look up locally first, and refer to global with an
+        # explicit qualifier like "root.path.to.config"
+        if root && enable_expansion
           value = root[name]
         end
 
+        if value.nil?
+          value = blk.call(name)
+        end
+
         if value.kind_of?(self.class) && value.root.nil?
           value.root = root || self
         end
 
         enable_expansion ? expand(value) : value
       end
 
+      def expanding_reader(key)
+        expand_results(key) {|k| orig_reader(k) }
+      end
+
+      def fetch(key, *args)
+        expand_results(key) {|k| super(k, *args) }
+      end
+
       def error_resolver
         @error_resolver || root.try(:error_resolver)
       end
@@ -52,28 +67,34 @@ def to_a
         self.collect {|k, v| [k, v]}
       end
 
+      def format_error(msg, expr, ex=nil)
+        file, line = nil, nil
+        if error_resolver
+          file, line = error_resolver.call(expr)
+        end
+        file_msg = file.nil? ? "" : " in #{File.basename(file)}:#{line}"
+        msg = "#{msg} '#{expr}'#{file_msg}"
+        if ex
+          msg +=  " => #{ex.class} #{ex.message}"
+        end
+        return msg
+      end
+
       def expand_string(obj)
         result = obj
         result.scan(INTERP_PATTERN).each do |substr, statement|
           # TODO: add an explicit check for cycles instead of relying on Stack error
-          if statement =~ /^[\w\.\[\]]$/
-            val = notation_get(statement)
-          else
+          begin
             # TODO: be consistent with dot notation between eval and
             # notation_get.  eval ends up calling Hashie method_missing,
             # which returns nil if a key doesn't exist, causing a nil
             # exception for next item in chain, while notation_get returns
             # nil gracefully for the entire chain (preferred)
-            begin
-              val = eval(statement, binding, __FILE__)
-            rescue => e
-              file, line = nil, nil
-              if error_resolver
-                file, line = error_resolver.call(substr)
-              end
-              file_msg = file.nil? ? "" : " in #{File.basename(file)}:#{line}"
-              raise RuntimeError.new("Failing config statement '#{substr}'#{file_msg} => #{e.class} #{e.message}")
-            end
+            val = eval(statement, binding, __FILE__)
+          rescue SystemStackError => e
+            raise ConfigInterpolationError.new(format_error("Cycle in interpolated config", substr))
+          rescue StandardError => e
+            raise ConfigInterpolationError.new(format_error("Failing config statement", substr, e))
           end
           result = result.sub(substr, val.to_s)
         end

spec/integration/initial_setup_spec.rb

@@ -40,6 +40,26 @@ def atmos(*args, output_on_fail: true, allow_fail: false, stdin_data: nil)
       end
     end
 
+    it "it catches and formats errors" do
+      within_construct do |c|
+        c.file('config/atmos.yml', 'foo: "#{fail}"')
+
+        output = atmos "config", allow_fail: true, output_on_fail: false
+        expect(output).to match(/Failing config statement/)
+        expect(output).to_not match(/\.rb:\d+:in /) # backtrace
+      end
+    end
+
+    it "it shows full trace in debug" do
+      within_construct do |c|
+        c.file('config/atmos.yml', 'foo: "#{fail}"')
+
+        output = atmos "--debug", "config", allow_fail: true, output_on_fail: false
+        expect(output).to match(/Failing config statement/)
+        expect(output).to match(/\.rb:\d+:in /) # backtrace
+      end
+    end
+
   end
 
   describe "new repo" do

spec/settings_hash_spec.rb

@@ -246,7 +246,7 @@ def create(*args)
 
         it "prevents cycles in interpolation" do
           config = create(foo: '#{baz}', baz: '#{foo}')
-          expect { config["foo"] }.to raise_error(SystemStackError)
+          expect { config["foo"] }.to raise_error(SimplyGenius::Atmos::Exceptions::ConfigInterpolationError)
         end
 
         it "handles multi eval" do
@@ -269,10 +269,18 @@ def create(*args)
           config = create({baz: '#{1/0}'})
           config.error_resolver = ->(s) { return "atmos.yml", 57 }
           expect{config["baz"]}.
-              to raise_error(RuntimeError,
+              to raise_error(SimplyGenius::Atmos::Exceptions::ConfigInterpolationError,
                              /Failing config statement '\#{1\/0}' in atmos.yml:57 => ZeroDivisionError.*/)
         end
 
+        it "raises when interpolating a non-existant path" do
+          config = create(foo: '#{bar.baz.boo}')
+          expect{config.foo}.
+              to raise_error(SimplyGenius::Atmos::Exceptions::ConfigInterpolationError,
+                            /Failing config statement '\#{bar.baz.boo}' => NoMethodError undefined method `baz' for nil:NilClass/)
+        end
+
+
         it "handles truthy" do
           config = create(foo: true, bar: false,
                                          baz: '#{foo}', bum: '#{bar}',
@@ -310,12 +318,22 @@ def create(*args)
           expect(config["baz"]).to eq("1")
         end
 
-        it "tries lookups from root if not found in current hash level" do
-          config = create(foo: {bar: "baz", bum: '#{bar}', hum: '#{foo.bar}'}, blah: "bah")
-          expect(config.foo.bum).to eq("baz")
+        it "looks up from root deeply" do
+          config = create(foo: {bar: "baz", bum: '#{bar}', hum: '#{foo.bar}'})
           expect(config.foo.hum).to eq("baz")
         end
 
+        it "looks up from root first" do
+          config = create(foo: {bar: "hum", baz: '#{bar}', hum: 'dum', bum: '#{hum}'}, bar: "bah")
+          expect(config.foo.baz).to eq("bah")
+          expect(config.foo.bum).to eq("dum")
+        end
+
+        it "expands for notation_get" do
+          config = create(foo: {bar: "baz", boo: '#{bar}'})
+          expect(config.notation_get("foo.boo")).to eq("baz")
+        end
+
       end
 
     end