Switching Facter to using the new loader.

This should make it possible to move all facts to separate
files and only load them on demand.

7 files changed, 153 insertions, 92 deletions

diff --git a/lib/facter.rb b/lib/facter.rb
index 0f00f7d..91588d3 100644
--- a/lib/facter.rb
+++ b/lib/facter.rb
@@ -77,11 +77,18 @@ module Facter
     end
 
     class << self
-        [:add, :each, :fact, :flush, :list, :to_hash, :value].each do |method|
+        [:add, :fact, :flush, :list, :value].each do |method|
             define_method(method) do |*args|
                 collection.send(method, *args)
             end
         end
+
+        [:list, :to_hash].each do |method|
+            define_method(method) do |*args|
+                collection.load_all
+                collection.send(method, *args)
+            end
+        end
     end
 
 
@@ -91,6 +98,15 @@ module Facter
         collection.add(name, options, &block)
     end
 
+    def self.each
+        # Make sure all facts are loaded.
+        collection.load_all
+
+        collection.each do |*args|
+            yield(*args)
+        end
+    end
+
     class << self
         # Allow users to call fact names directly on the Facter class,
         # either retrieving the value or comparing it to an existing value.
@@ -189,49 +205,7 @@ module Facter
             end
         end
 
-        locals = []
-
-        # Now find all our loadable facts
-        factdirs = [] # All the places to check for facts
-        
-        # See if we can find any other facts in the regular Ruby lib
-        # paths
-        $:.each do |dir|
-            fdir = File.join(dir, "facter")
-            if FileTest.exists?(fdir) and FileTest.directory?(fdir)
-                factdirs.push(fdir)
-            end
-        end
-        # Also check anything in 'FACTERLIB'
-        if ENV['FACTERLIB']
-            ENV['FACTERLIB'].split(":").each do |fdir|
-                factdirs.push(fdir)
-            end
-        end
-        factdirs.each do |fdir|
-            Dir.glob("#{fdir}/*.rb").each do |file|
-                # Load here, rather than require, because otherwise
-                # the facts won't get reloaded if someone calls
-                # "loadfacts".  Really only important in testing, but,
-                # well, it's important in testing.
-                begin
-                    load file
-                rescue => detail
-                    warn "Could not load %s: %s" %
-                        [file, detail]
-                end
-            end
-        end
-        
-
-        # Now try to get facts from the environment
-        ENV.each do |name, value|
-            if name =~ /^facter_?(\w+)$/i
-                Facter.add($1) do
-                    setcode { value }
-                end
-            end
-        end
+        collection.load_all
     end
 
     Facter.loadfacts
diff --git a/lib/facter/util/collection.rb b/lib/facter/util/collection.rb
index f76b578..267424a 100644
--- a/lib/facter/util/collection.rb
+++ b/lib/facter/util/collection.rb
@@ -1,5 +1,6 @@
 require 'facter'
 require 'facter/util/fact'
+require 'facter/util/loader'
 
 # Manage which facts exist and how we access them.  Largely just a wrapper
 # around a hash of facts.
@@ -39,7 +40,11 @@ class Facter::Util::Collection
 
     # Return a fact by name.
     def fact(name)
-        @facts[canonize(name)]
+        name = canonize(name)
+
+        loader.load(name) unless @facts[name]
+
+        return @facts[name]
     end
 
     # Flush all cached values.
@@ -56,6 +61,19 @@ class Facter::Util::Collection
         return @facts.keys
     end
 
+    # Load all known facts.
+    def load_all
+        loader.load_all
+    end
+
+    # The thing that loads facts if we don't have them.
+    def loader
+        unless defined?(@loader)
+            @loader = Facter::Util::Loader.new
+        end
+        @loader
+    end
+
     # Return a hash of all of our facts.
     def to_hash
         @facts.inject({}) do |h, ary|
diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index 7031172..52b5f07 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -13,8 +13,7 @@ class Facter::Util::Loader
             # Load individual files
             file = File.join(dir, filename)
 
-            # We have to specify Kernel.load, because we have a load method.
-            Kernel.load(file) if FileTest.exist?(file)
+            load_file(file) if FileTest.exist?(file)
 
             # And load any directories matching the name
             factdir = File.join(dir, shortname)
@@ -24,18 +23,24 @@ class Facter::Util::Loader
 
     # Load all facts from all directories.
     def load_all
+        return if defined?(@loaded_all)
+
         load_env
 
         search_path.each do |dir|
+            next unless FileTest.directory?(dir)
+
             Dir.entries(dir).each do |file|
                 path = File.join(dir, file)
                 if File.directory?(path)
                     load_dir(path)
                 elsif file =~ /\.rb$/
-                    Kernel.load(File.join(dir, file))
+                    load_file(File.join(dir, file))
                 end
             end
         end
+
+        @loaded_all = true
     end
 
     # The list of directories we're going to search through for facts.
@@ -53,56 +58,21 @@ class Facter::Util::Loader
         result
     end
 
-    def old_stuff
-        # See if we can find any other facts in the regular Ruby lib
-        # paths
-        $:.each do |dir|
-            fdir = File.join(dir, "facter")
-            if FileTest.exists?(fdir) and FileTest.directory?(fdir)
-                factdirs.push(fdir)
-            end
-        end
-        # Also check anything in 'FACTERLIB'
-        if ENV['FACTERLIB']
-            ENV['FACTERLIB'].split(":").each do |fdir|
-                factdirs.push(fdir)
-            end
-        end
-        factdirs.each do |fdir|
-            Dir.glob("#{fdir}/*.rb").each do |file|
-                # Load here, rather than require, because otherwise
-                # the facts won't get reloaded if someone calls
-                # "loadfacts".  Really only important in testing, but,
-                # well, it's important in testing.
-                begin
-                    load file
-                rescue => detail
-                    warn "Could not load %s: %s" %
-                        [file, detail]
-                end
-            end
-        end
-        
-
-        # Now try to get facts from the environment
-        ENV.each do |name, value|
-            if name =~ /^facter_?(\w+)$/i
-                Facter.add($1) do
-                    setcode { value }
-                end
-            end
-        end
-    end
-
     private
 
     def load_dir(dir)
-        return if dir =~ /\/util$/
+        return if dir =~ /\/\.+$/ or dir =~ /\/util$/ or dir =~ /\/lib$/
+
         Dir.entries(dir).find_all { |f| f =~ /\.rb$/ }.each do |file|
-            Kernel.load(File.join(dir, file))
+            load_file(File.join(dir, file))
         end
     end
 
+    def load_file(file)
+        # We have to specify Kernel.load, because we have a load method.
+        Kernel.load(file)
+    end
+
     # Load facts from the environment.  If no name is provided,
     # all will be loaded.
     def load_env(fact = nil)
diff --git a/spec/integration/facter.rb b/spec/integration/facter.rb
index e1f2025..79a1f0f 100755
--- a/spec/integration/facter.rb
+++ b/spec/integration/facter.rb
@@ -5,7 +5,6 @@ require File.dirname(__FILE__) + '/../spec_helper'
 describe Facter do
     before do
         Facter.reset
-        Facter.loadfacts
     end
 
     after do
diff --git a/spec/unit/facter.rb b/spec/unit/facter.rb
index 53796ad..59d10d4 100755
--- a/spec/unit/facter.rb
+++ b/spec/unit/facter.rb
@@ -31,11 +31,23 @@ describe Facter do
         Facter.list
     end
 
+    it "should load all facts when listing" do
+        Facter.collection.expects(:load_all)
+        Facter.collection.stubs(:list)
+        Facter.list
+    end
+
     it "should delegate the :to_hash method to the collection" do
         Facter.collection.expects(:to_hash)
         Facter.to_hash
     end
 
+    it "should load all facts when calling :to_hash" do
+        Facter.collection.expects(:load_all)
+        Facter.collection.stubs(:to_hash)
+        Facter.to_hash
+    end
+
     it "should delegate the :value method to the collection" do
         Facter.collection.expects(:value)
         Facter.value
@@ -46,6 +58,20 @@ describe Facter do
         Facter.each
     end
 
+    it "should load all facts when calling :each" do
+        Facter.collection.expects(:load_all)
+        Facter.collection.stubs(:each)
+        Facter.each
+    end
+
+    it "should yield to the block when using :each" do
+        Facter.collection.stubs(:load_all)
+        Facter.collection.stubs(:each).yields "foo"
+        result = []
+        Facter.each { |f| result << f }
+        result.should == %w{foo}
+    end
+
     describe "when provided code as a string" do
         it "should execute the code in the shell" do
             Facter.add("shell_testing") do
diff --git a/spec/unit/util/collection.rb b/spec/unit/util/collection.rb
index e88de06..9ea2e9d 100755
--- a/spec/unit/util/collection.rb
+++ b/spec/unit/util/collection.rb
@@ -9,6 +9,29 @@ describe Facter::Util::Collection do
         Facter::Util::Collection.new.should respond_to(:add)
     end
 
+    it "should have a method for returning a loader" do
+        Facter::Util::Collection.new.should respond_to(:loader)
+    end
+
+    it "should use an instance of the Loader class as its loader" do
+        Facter::Util::Collection.new.loader.should be_instance_of(Facter::Util::Loader)
+    end
+
+    it "should cache its loader" do
+        coll = Facter::Util::Collection.new
+        coll.loader.should equal(coll.loader)
+    end
+
+    it "should have a method for loading all facts" do
+        Facter::Util::Collection.new.should respond_to(:load_all)
+    end
+
+    it "should delegate its load_all method to its loader" do
+        coll = Facter::Util::Collection.new
+        coll.loader.expects(:load_all)
+        coll.load_all
+    end
+
     describe "when adding facts" do
         before do
             @coll = Facter::Util::Collection.new
@@ -55,6 +78,16 @@ describe Facter::Util::Collection do
         it "should treat strings and symbols equivalently" do
             @coll.fact(:yayness).should equal(@fact)
         end
+
+        it "should use its loader to try to load the fact if no fact can be found" do
+            @coll.loader.expects(:load).with(:testing)
+            @coll.fact("testing")
+        end
+
+        it "should return nil if it cannot find or load the fact" do
+            @coll.loader.expects(:load).with(:testing)
+            @coll.fact("testing").should be_nil
+        end
     end
 
     it "should have a method for returning a fact's value" do
diff --git a/spec/unit/util/loader.rb b/spec/unit/util/loader.rb
index e6a7f17..690ec03 100755
--- a/spec/unit/util/loader.rb
+++ b/spec/unit/util/loader.rb
@@ -149,6 +149,18 @@ describe Facter::Util::Loader do
         before do
             @loader = Facter::Util::Loader.new
             @loader.stubs(:search_path).returns []
+
+            FileTest.stubs(:directory?).returns true
+        end
+
+        it "should skip directories that do not exist" do
+            @loader.expects(:search_path).returns %w{/one/dir}
+
+            FileTest.expects(:directory?).with("/one/dir").returns false
+
+            Dir.expects(:entries).with("/one/dir").never
+
+            @loader.load_all
         end
 
         it "should load all files in all search paths" do
@@ -190,6 +202,29 @@ describe Facter::Util::Loader do
             @loader.load_all
         end
 
+        it "should not load files in a lib subdirectory" do
+            @loader.expects(:search_path).returns %w{/one/dir}
+
+            Dir.expects(:entries).with("/one/dir").returns %w{lib}
+
+            File.expects(:directory?).with("/one/dir/lib").returns true
+
+            Dir.expects(:entries).with("/one/dir/lib").never
+
+            @loader.load_all
+        end
+
+        it "should not load files in '.' or '..'" do
+            @loader.expects(:search_path).returns %w{/one/dir}
+
+            Dir.expects(:entries).with("/one/dir").returns %w{. ..}
+
+            File.expects(:entries).with("/one/dir/.").never
+            File.expects(:entries).with("/one/dir/..").never
+
+            @loader.load_all
+        end
+
         it "should load all facts from the environment" do
             Facter.expects(:add).with('one')
             Facter.expects(:add).with('two')
@@ -198,5 +233,11 @@ describe Facter::Util::Loader do
                 @loader.load_all
             end
         end
+
+        it "should only load all facts one time" do
+            @loader.expects(:load_env).once
+            @loader.load_all
+            @loader.load_all
+        end
     end
 end