Added some documentation and cleaned up unecessary TODOs. Added first round of specs for the new classes.

4 files changed, 32 insertions, 22 deletions

diff --git a/lib/chef/resource_collection.rb b/lib/chef/resource_collection.rb
index 60e322af44..2e202bd6eb 100644
--- a/lib/chef/resource_collection.rb
+++ b/lib/chef/resource_collection.rb
@@ -20,6 +20,7 @@
 require 'chef/resource_collection/resource_set'
 require 'chef/resource_collection/resource_list'
 require 'chef/resource_collection/resource_collection_serialization'
+require 'chef/log'
 
 ##
 # ResourceCollection currently handles two tasks:
@@ -56,36 +57,34 @@ class Chef
 
     # @param insert_at_index [Integer] Location to insert resources
     # @param resources [Chef::Resource] Resources to insert
-    # @depreciated Callers should use the insert method above and loop through their resources as necessary
+    # @deprecated Callers should use the insert method above and loop through their resources as necessary
     def insert_at(insert_at_index, *resources)
+      Chef::Log.warn("`insert_at` is deprecated, use `insert` with the `at_location` parameter")
       @resource_list.insert_at(insert_at_index, *resources)
       resources.each do |resource|
         @resource_set.insert_as(resource)
       end
     end
 
-    # @depreciated
+    # @deprecated
     def []=(index, resource)
+      Chef::Log.warn("`[]=` is deprecated, use `insert` with the `at_location` parameter")
       @resource_list[index] = resource
       @resource_set.insert_as(resource)
     end
 
-    # @depreciated
+    # @deprecated
     def <<(*resources)
+      Chef::Log.warn("`<<` is deprecated, use `insert`")
       resources.flatten.each do |res|
         insert(res)
       end
       self
     end
 
-    # @depreciated
+    # @deprecated
     alias_method :push, :<<
 
-    # TODO when there were 2 resources with the same key in resource_set, how do we handle notifications since they get copied?
-    # Did the old class only keep the last seen reference?
-
-    # TODO do we need to implement a dup method?  Run_context was shallowly copying resource_collection before
-
     # Read-only methods are simple to proxy - doing that below
 
     RESOURCE_LIST_METHODS = Enumerable.instance_methods +
diff --git a/lib/chef/resource_collection/resource_collection_serialization.rb b/lib/chef/resource_collection/resource_collection_serialization.rb
index 8c11b9beeb..3651fb2a2a 100644
--- a/lib/chef/resource_collection/resource_collection_serialization.rb
+++ b/lib/chef/resource_collection/resource_collection_serialization.rb
@@ -16,7 +16,6 @@
 # limitations under the License.
 #
 class Chef
-  # TODO move into subfolder until we promote these to top level classes
   class ResourceCollection
     module ResourceCollectionSerialization
       # Serialize this object as a hash
diff --git a/lib/chef/resource_collection/resource_list.rb b/lib/chef/resource_collection/resource_list.rb
index 50b5a41e58..56bf89f1e4 100644
--- a/lib/chef/resource_collection/resource_list.rb
+++ b/lib/chef/resource_collection/resource_list.rb
@@ -20,6 +20,8 @@ require 'chef/resource'
 require 'chef/resource_collection/stepable_iterator'
 require 'chef/resource_collection/resource_collection_serialization'
 
+# This class keeps the list of all known Resources in the order they are to be executed in.  It also keeps a pointer
+# to the most recently executed resource so we can add resources-to-execute after this point.
 class Chef
   class ResourceCollection
     class ResourceList
@@ -33,28 +35,41 @@ class Chef
         @insert_after_idx = nil
       end
 
-      # TODO the differences between these 2 insert methods is very confusing
+      # @param resource [Chef::Resource] The resource to insert
+      # If @insert_after_idx is nil, we are not currently executing a converge so the Resource is appended to the
+      # end of the list.  If @insert_after_idx is NOT nil, we ARE currently executing a converge so the resource
+      # is inserted into the middle of the list after the last resource that was converged.  If it is called multiple
+      # times (when an LWRP contains multiple resources) it keeps track of that.  See this example ResourceList:
+      # [File1, LWRP1, File2] # The iterator starts and points to File1.  It is executed and @insert_after_idx=0
+      # [File1, LWRP1, File2] # The iterator moves to LWRP1.  It is executed and @insert_after_idx=1
+      # [File1, LWRP1, Service1, File2] # The LWRP execution inserts Service1 and @insert_after_idx=2
+      # [File1, LWRP1, Service1, Service2, File2] # The LWRP inserts Service2 and @insert_after_idx=3.  The LWRP
+      #     finishes executing
+      # [File1, LWRP1, Service1, Service2, File2] # The iterator moves to Service1 since it is the next non-executed
+      #     resource.  The execute_each_resource call below resets @insert_after_idx=2
+      # If Service1 was another LWRP, it would insert its resources between Service1 and Service2.  The iterator keeps
+      # track of executed resources and @insert_after_idx keeps track of where the next resource to insert should be.
       def insert(resource)
+        is_chef_resource!(resource)
         if @insert_after_idx
-          # in the middle of executing a run, so any resources inserted now should
-          # be placed after the most recent addition done by the currently executing
-          # resource
-          insert_at(@insert_after_idx += 1, resource)
+          @resources.insert(@insert_after_idx += 1, resource)
         else
-          is_chef_resource!(resource)
           @resources << resource
         end
       end
 
-      # TODO this did not adjust @insert_after_idx in the old class - add test case and ask JohnK
+      # @param index [Integer] location in the array to insert the resources
+      # @param resources [Array of Chef::Resource] Resources to insert
+      # Locate the index indicated and insert all resources, pushing any remaining resources further down in the array.
       def insert_at(index, *resources)
         resources.each do |resource|
           is_chef_resource!(resource)
         end
         @resources.insert(index, *resources)
+        @insert_after_idx += resources.size unless @insert_after_idx.nil?
       end
 
-      # @depreciated
+      # @deprecated - can be removed when it is removed from resource_collection.rb
       def []=(index, resource)
         @resources[index] = resource
       end
@@ -73,8 +88,6 @@ class Chef
         end
       end
 
-      # TODO I would like to rename this to something that illustrates it sets the @insert_after_idx variable, then alias this old name
-      # TODO or perhaps refactor it to have 2 pointers - 1 for the end of the list and 1 for resources we have processed
       #   so far, and then move that logic up into the ResourceCollection class to simplify this class
       def execute_each_resource(&resource_exec_block)
         @iterator = ResourceCollection::StepableIterator.for_collection(@resources)
diff --git a/lib/chef/resource_collection/resource_set.rb b/lib/chef/resource_collection/resource_set.rb
index 65e47972cd..e3287951d7 100644
--- a/lib/chef/resource_collection/resource_set.rb
+++ b/lib/chef/resource_collection/resource_set.rb
@@ -20,7 +20,6 @@ require 'chef/resource'
 require 'chef/resource_collection/resource_collection_serialization'
 
 class Chef
-  # TODO move into subfolder until we promote these to top level classes
   class ResourceCollection
     class ResourceSet
       include ResourceCollection::ResourceCollectionSerialization
@@ -93,7 +92,7 @@ class Chef
         flat_results.length == 1 ? flat_results[0] : flat_results
       end
 
-      # @depreciated
+      # @deprecated
       # resources is a poorly named, but we have to maintain it for back
       # compat.
       alias_method :resources, :find