Since this patch has been sitting on the list with no comments for a while, Jesse and I took a stab at reviewing it last week. Comments below:
On Wed, Sep 8, 2010 at 11:53 AM, Paul Berry <[email protected]> wrote: > # Clear our graph. > def clear > - @vertices.each { |vertex, wrapper| wrapper.clear } > - @vertices.clear > - @edges.clear > - end > - > - # Which resources a given resource depends upon. > - def dependents(resource) > - tree_from_vertex(resource).keys > + @in_to.each { |k,v| v.clear } > + @out_from.each { |k,v| v.clear } > The above two lines seem unnecessary since they are followed by clearing @in_to and @out_from. Was this an attempt to avoid shedding GC loops? Because if so there is actually no benefit, since the sub-hashes contained within @in_to and @out_from are not referred to by anything else. > + @in_to.clear > + @out_from.clear > + @upstream_from.clear > + @downstream_from.clear > end > > # Which resources depend upon the given resource. > def dependencies(resource) > - # Cache the reversal graph, because it's somewhat expensive > - # to create. > - @reversal ||= reversal > - # Strangely, it's significantly faster to search a reversed > - # tree in the :out direction than to search a normal tree > - # in the :in direction. > - @reversal.tree_from_vertex(resource, :out).keys > + vertex?(resource) ? upstream_from_vertex(resource).keys : [] > Looks like an indentation error--there should be one more space before "vertex". > + end > + > + def dependents(resource) > + vertex?(resource) ? downstream_from_vertex(resource).keys : [] > Indentation error here too. > end > > # Whether our graph is directed. Always true. Used to produce dot > files. > @@ -209,103 +158,87 @@ class Puppet::SimpleGraph > > # Add a new vertex to the graph. > def add_vertex(vertex) > - @reversal = nil > - return false if vertex?(vertex) > - setup_vertex(vertex) > - true # don't return the VertexWrapper instance. > + @upstream_from.clear > + @downstream_from.clear > There's no need to clear @upstream_from and @downstream_from, because adding a lone vertex can't change those relationships. > + @in_to[vertex] ||= {} > + @out_from[vertex] ||= {} > end > > # Find a matching edge. Note that this only finds the first edge, > # not all of them or whatever. > def edge(source, target) > - @edges.each_with_index { |test_edge, index| return test_edge if > test_edge.source == source and test_edge.target == target } > + edge?(source,target) && @out_from[source][target].first > end > > def edge_label(source, target) > - return nil unless edge = edge(source, target) > - edge.label > + (e = edge(source, target)) && e.label > end > It's hard to imagine circumstances in which the above two methods would be useful, because if there are edges between the source and target, they only return information about one of the edges. Also, it looks like nobody calls these methods. Can we eliminate them? > - > - public > - > -# # For some reason, unconnected vertices do not show up in > -# # this graph. > -# def to_jpg(path, name) > -# gv = vertices > -# Dir.chdir(path) do > -# induced_subgraph(gv).write_to_graphic_file('jpg', name) > -# end > -# end > Since this was the only method that called write_to_graphic_file(), we should eliminate it too. > @@ -381,6 +314,14 @@ class Puppet::SimpleGraph > predecessor > end > > + def downstream_from_vertex(v) > + @downstream_from[v] || @out_from[v].keys.inject(@downstream_from[v] = > {}) { |result,node| result.update(node=>1).update > downstream_from_vertex(node) } > + end > + > + def upstream_from_vertex(v) > + @upstream_from[v] || @in_to[v].keys.inject( @upstream_from[v] = > {}) { |result,node| result.update(node=>1).update upstream_from_vertex(node) > } > + end > + > These two methods are very difficult to read. Is there a good reason not to do something like this? def downstream_from_vertex(v) return @downstream_from[v] if @downstream_from[v] result = @downstream_from[v] = {} @out_from.keys.each do |node| result[node] = 1 result.update(downstream_from_vertex(node)) end result end Also, these methods are only used by dependencies and dependents, above, and those methods only care about the keys of the returned hashes. It seems like it would be clearer to implement them using Set objects rather than hashes. > diff --git a/spec/unit/util/zaml_spec.rb b/spec/unit/util/zaml_spec.rb > index 4de57e6..38a9541 100644 > --- a/spec/unit/util/zaml_spec.rb > +++ b/spec/unit/util/zaml_spec.rb > @@ -36,3 +36,28 @@ describe "Pure ruby yaml implementation" do > } > end > > The tests below would make more sense in spec/unit/util/monkey_patches_spec.rb since the code they test is in monkey_patches.rb. > +describe "yaml deserialization" do > + it "should call yaml_initialize when deserializing objects that have > that method defined" do > + class Puppet::TestYamlInitializeClass > + attr_reader :foo > + > + def yaml_initialize(tag, var) > + var.should == {'foo' => 100} > + instance_variable_defined?(:@hook_called).should == false > The above line was a mistake--it was intending to verify that there were no instance variables defined at the time yaml_initialize was called. It should be replaced with something like "instance_variables.should == []". > + @foo = 200 > + end > + end > + > + obj = YAML.load("--- !ruby/object:Puppet::TestYamlInitializeClass\n > foo: 100") > + obj.foo.should == 200 > + end > + > + it "should not call yaml_initialize if not defined" do > + class Puppet::TestYamlNonInitializeClass > + attr_reader :foo > + end > + > + obj = YAML.load("--- !ruby/object:Puppet::TestYamlNonInitializeClass\n > foo: 100") > + obj.foo.should == 100 > + end > +end -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
