lots of refactoring

git-svn-id: https://reductivelabs.com/svn/puppet/library/trunk@372 980ebf18-57e1-0310-9a29-db15c13687c0

3 files changed, 161 insertions, 165 deletions

diff --git a/lib/puppet/metric.rb b/lib/puppet/metric.rb
index 218befb2e..4b606b87d 100644
--- a/lib/puppet/metric.rb
+++ b/lib/puppet/metric.rb
@@ -28,7 +28,7 @@ module Puppet
             Puppet::Type.eachtype { |type|
                 type.each { |instance|
                     @@typemetrics[type][:total] += 1
-                    if instance.managed
+                    if instance.managed?
                         @@typemetrics[type][:managed] += 1
                     end
                 }
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index a7c3eaeb6..df71f352e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -227,7 +227,7 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # remove all type instances
+    # remove all type instances; this is mostly only useful for testing
     def self.allclear
         @@typeary.each { |subtype|
             debug "Clearing %s of objects" % subtype
@@ -237,7 +237,7 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # per-type clearance
+    # remove all of the instances of a single type
     def self.clear
         if defined? @objects
             @objects.clear
@@ -246,6 +246,7 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
+    # iterate across each of the type's instances
     def self.each
         return unless defined? @objects
         @objects.each { |name,instance|
@@ -255,13 +256,14 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
+    # does the type have an object with the given name?
     def self.has_key?(name)
         return @objects.has_key?(name)
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # all objects total
+    # add an object to the master list of Type instances
     def self.push(object)
         @@allobjects.push object
         #debug("adding %s of type %s to master list" %
@@ -279,6 +281,7 @@ class Type < Puppet::Element
     public
 
     #---------------------------------------------------------------
+    # build a per-Type hash, mapping the states to their names
     def self.buildstatehash
         unless defined? @validstates
             @validstates = Hash.new(false)
@@ -299,7 +302,8 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # this is probably only used by FileRecord objects
+    # set the parameters for a type; probably only used by FileRecord
+    # objects
     def self.parameters=(params)
         debug "setting parameters to [%s]" % params.join(" ")
         @parameters = params.collect { |param|
@@ -313,7 +317,31 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # this abstracts accessing parameters and states, and normalizes
+    # does the name reflect a valid state?
+    def self.validstate?(name)
+        unless @validstates.length == @states.length
+            self.buildstatehash
+        end
+        if @validstates.include?(name)
+            return @validstates[name]
+        else
+            return false
+        end
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
+    # does the name reflect a valid parameter?
+    def self.validparameter?(name)
+        unless defined? @parameters
+            raise "Class %s has not defined parameters" % self
+        end
+        return @parameters.include?(name)
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
+    # abstract accessing parameters and states, and normalize
     # access to always be symbols, not strings
     # XXX this returns a _value_, not an object
     # if you want a state object, use <type>.state(<state>)
@@ -341,7 +369,7 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # this abstracts setting parameters and states, and normalizes
+    # abstract setting parameters and states, and normalize
     # access to always be symbols, not strings
     def []=(name,value)
         mname = name
@@ -379,7 +407,8 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # removing states
+    # remove a state from the object; useful in testing or in cleanup
+    # when an error has been encountered
     def delete(attr)
         if @states.has_key?(attr)
             @states.delete(attr)
@@ -390,39 +419,92 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def state(name)
-        return @states[name]
+    # iterate across all children, and then iterate across states
+    # we do children first so we're sure that all dependent objects
+    # are checked first
+    # we ignore parameters here, because they only modify how work gets
+    # done, they don't ever actually result in work specifically
+    def each
+        # we want to return the states in the order that each type
+        # specifies it, because it may (as in the case of File#create)
+        # be important
+        @children.each { |child|
+            yield child
+        }
+        self.eachstate { |state|
+            yield state
+        }
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def parameter(name)
-        return @parameters[name]
+    # iterate across the existing states
+    def eachstate
+        states.each { |state|
+            yield state
+        }
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def self.validstate?(name)
-        unless @validstates.length == @states.length
-            self.buildstatehash
-        end
-        if @validstates.include?(name)
-            return @validstates[name]
+    # is the instance a managed instance?  A 'yes' here means that
+    # the instance was created from the language, vs. being created
+    # in order resolve other questions, such as finding a package
+    # in a list
+    def managed?
+        if defined? @managed
+            return @managed
         else
-            return false
+            @managed = false
+            states.each { |state|
+                if state.should
+                    @managed = true
+                end
+            }
+            return @managed
         end
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def self.validparameter?(name)
-        unless defined? @parameters
-            raise "Class %s has not defined parameters" % self
+    # return the value of a parameter
+    def parameter(name)
+        return @parameters[name]
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
+    def push(*child)
+        @children.push(*child)
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
+    # return an actual type by name; to return the value, use 'inst[name]'
+    def state(name)
+        return @states[name]
+    end
+    #---------------------------------------------------------------
+
+    private
+
+    #---------------------------------------------------------------
+    def states
+        debug "%s has %s states" % [self,@states.length]
+        tmpstates = []
+        self.class.states.each { |state|
+            if @states.include?(state.name)
+                tmpstates.push(@states[state.name])
+            end
+        }
+        unless tmpstates.length == @states.length
+            raise "Something went very wrong with tmpstates creation"
         end
-        return @parameters.include?(name)
+        return tmpstates
     end
     #---------------------------------------------------------------
 
+
     #---------------------------------------------------------------
     #---------------------------------------------------------------
     # instance methods related to instance intrinsics
@@ -433,6 +515,7 @@ class Type < Puppet::Element
     public
 
     #---------------------------------------------------------------
+    # initialize the type instance
     def initialize(hash)
         @children = []
         @evalcount = 0
@@ -493,50 +576,13 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # return the full path to us, for logging and rollback
-    # some classes (e.g., FileTypeRecords) will have to override this
-    def path
-        return [self.path, self.name].flatten
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
-    # this might result from a state or from a parameter
+    # derive the instance name based on class.namevar
     def name
         return self[self.class.namevar]
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def retrieve
-        # it's important to use the method here, so we always get
-        # them back in the right order
-        states.collect { |state|
-            state.retrieve
-        }
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
-    def sync
-        events = self.collect { |child|
-            child.sync
-        }.reject { |event|
-            ! (event.is_a?(Symbol) or event.is_a?(String))
-        }.flatten
-
-        Puppet::Metric.addevents(self.class,self,events)
-        return events
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
-    def to_s
-        self.name
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
     # fix any namevar => param translations
     def nameclean(hash)
         # we have to set the name of our object before anything else,
@@ -561,77 +607,42 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    #---------------------------------------------------------------
-    # instance methods dealing with contained states
-    #---------------------------------------------------------------
-    #---------------------------------------------------------------
-
-    public
-
-    #---------------------------------------------------------------
-    def managed
-        if defined? @managed
-            return @managed
-        else
-            @managed = false
-            states.each { |state|
-                if state.should
-                    @managed = true
-                end
-            }
-            return @managed
-        end
+    # return the full path to us, for logging and rollback
+    # some classes (e.g., FileTypeRecords) will have to override this
+    def path
+        return [self.class, self.name].flatten
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    def eachstate
-        states.each { |state|
-            yield state
+    # retrieve the current value of all contained states
+    def retrieve
+        # it's important to use the method here, as it follows the order
+        # in which they're defined in the object
+        states.collect { |state|
+            state.retrieve
         }
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # iterate across all children, and then iterate across states
-    # we do children first so we're sure that all dependent objects
-    # are checked first
-    # we ignore parameters here, because they only modify how work gets
-    # done, they don't ever actually result in work specifically
-    def each
-        # we want to return the states in the order that each type
-        # specifies it, because it may (as in the case of File#create)
-        # be important
-        @children.each { |child|
-            yield child
-        }
-        self.eachstate { |state|
-            yield state
-        }
-    end
-    #---------------------------------------------------------------
+    # sync the changes to disk, and return the events generated by the changes
+    def sync
+        events = self.collect { |child|
+            child.sync
+        }.reject { |event|
+            ! (event.is_a?(Symbol) or event.is_a?(String))
+        }.flatten
 
-    #---------------------------------------------------------------
-    def push(*child)
-        @children.push(*child)
+        Puppet::Metric.addevents(self.class,self,events)
+        return events
     end
     #---------------------------------------------------------------
 
-    private
-
     #---------------------------------------------------------------
-    def states
-        debug "%s has %s states" % [self,@states.length]
-        tmpstates = []
-        self.class.states.each { |state|
-            if @states.include?(state.name)
-                tmpstates.push(@states[state.name])
-            end
-        }
-        unless tmpstates.length == @states.length
-            raise "Something went very wrong with tmpstates creation"
-        end
-        return tmpstates
+    # convert to a string
+    def to_s
+        self.name
     end
     #---------------------------------------------------------------
 
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index 64ed56293..7bbc22b8e 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -21,23 +21,24 @@ class State < Puppet::Element
     @virtual = true
 
     #---------------------------------------------------------------
+    # which event gets generated if this state change happens; not currently
+    # called
+    def self.generates
+        return @event
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
     # every state class must tell us what its name will be (as a symbol)
     # this determines how we will refer to the state during usage
     # e.g., the Owner state for Files might say its name is :owner;
     # this means that we can say "file[:owner] = 'yayness'"
-    def State.name
+    def self.name
         return @name
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # which event gets generated if this state change happens
-    def State.generates
-        return @event
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
     # if we're not in sync, return a statechange capable of putting us
     # in sync
     def evaluate
@@ -53,23 +54,7 @@ class State < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # return the full path to us, for logging and rollback
-    def path
-        return [@parent.path, self.name].flatten
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
-    # we aren't actually comparing the states themselves, we're only
-    # comparing the "should" value with the "is" value
-    def insync?
-        debug "%s value is '%s', should be '%s'" %
-            [self,self.is.inspect,self.should.inspect]
-        self.is == self.should
-    end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
+    # initialize our state
     def initialize(hash)
         @is = nil
 
@@ -87,29 +72,16 @@ class State < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # for testing whether we should actually do anything
-    def noop
-        unless defined? @noop
-            @noop = false
-        end
-        tmp = @noop || self.parent.noop || Puppet[:noop] || false
-        debug "noop is %s" % tmp
-        return tmp
+    # we aren't actually comparing the states themselves, we're only
+    # comparing the "should" value with the "is" value
+    def insync?
+        debug "%s value is '%s', should be '%s'" %
+            [self,self.is.inspect,self.should.inspect]
+        self.is == self.should
     end
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    #def refresh(transaction)
-    #    self.retrieve
-
-        # we definitely need some way to batch these refreshes, so a
-        # given object doesn't get refreshed multiple times in a single
-        # run
-    #    @parent.refresh
-    #end
-    #---------------------------------------------------------------
-
-    #---------------------------------------------------------------
     # each state class must define the name() method, and state instances
     # do not change that name
     # this implicitly means that a given object can only have one state
@@ -120,9 +92,22 @@ class State < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
-    # retrieve the current state from the running system
-    def retrieve
-        raise "'retrieve' method was not overridden by %s" % self.class
+    # for testing whether we should actually do anything
+    def noop
+        unless defined? @noop
+            @noop = false
+        end
+        tmp = @noop || self.parent.noop || Puppet[:noop] || false
+        debug "noop is %s" % tmp
+        return tmp
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
+    # return the full path to us, for logging and rollback; not currently
+    # used
+    def path
+        return [@parent.path, self.name].flatten
     end
     #---------------------------------------------------------------