diff options
| -rw-r--r-- | lib/puppet/interface.rb | 2 | ||||
| -rw-r--r-- | lib/puppet/interface/action.rb | 14 | ||||
| -rw-r--r-- | lib/puppet/interface/option_manager.rb | 29 | ||||
| -rwxr-xr-x | spec/shared_behaviours/things_that_declare_options.rb | 7 | ||||
| -rwxr-xr-x | spec/unit/interface/action_spec.rb | 285 |
5 files changed, 212 insertions, 125 deletions
diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb index ba68ac65b..eb376c4c5 100644 --- a/lib/puppet/interface.rb +++ b/lib/puppet/interface.rb @@ -139,6 +139,8 @@ class Puppet::Interface action.get_option(name).__decoration_name(type) end + methods.reverse! if type == :after + # Exceptions here should propagate up; this implements a hook we can use # reasonably for option validation. methods.each do |hook| diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index 464b2a738..96bb5b438 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -9,7 +9,10 @@ class Puppet::Interface::Action @name = name.to_sym attrs.each do |k, v| send("#{k}=", v) end - @options = {} + # @options collects the added options in the order they're declared. + # @options_hash collects the options keyed by alias for quick lookups. + @options = [] + @options_hash = {} @when_rendering = {} end @@ -211,22 +214,23 @@ WRAPPER end option.aliases.each do |name| - @options[name] = option + @options << name + @options_hash[name] = option end option end def option?(name) - @options.include? name.to_sym + @options_hash.include? name.to_sym end def options - (@options.keys + @face.options).sort + @face.options + @options end def get_option(name, with_inherited_options = true) - option = @options[name.to_sym] + option = @options_hash[name.to_sym] if option.nil? and with_inherited_options option = @face.get_option(name) end diff --git a/lib/puppet/interface/option_manager.rb b/lib/puppet/interface/option_manager.rb index d42359c07..326a91d92 100644 --- a/lib/puppet/interface/option_manager.rb +++ b/lib/puppet/interface/option_manager.rb @@ -8,6 +8,11 @@ module Puppet::Interface::OptionManager end def add_option(option) + # @options collects the added options in the order they're declared. + # @options_hash collects the options keyed by alias for quick lookups. + @options ||= [] + @options_hash ||= {} + option.aliases.each do |name| if conflict = get_option(name) then raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}" @@ -21,25 +26,30 @@ module Puppet::Interface::OptionManager end end - option.aliases.each { |name| @options[name] = option } - option + option.aliases.each do |name| + @options << name + @options_hash[name] = option + end + + return option end def options - @options ||= {} - result = @options.keys + result = (@options ||= []) if self.is_a?(Class) and superclass.respond_to?(:options) - result += superclass.options + result = superclass.options + result elsif self.class.respond_to?(:options) - result += self.class.options + result = self.class.options + result end - result.sort + + return result end def get_option(name, with_inherited_options = true) - @options ||= {} - result = @options[name.to_sym] + @options_hash ||= {} + + result = @options_hash[name.to_sym] if result.nil? and with_inherited_options then if self.is_a?(Class) and superclass.respond_to?(:get_option) result = superclass.get_option(name) @@ -47,6 +57,7 @@ module Puppet::Interface::OptionManager result = self.class.get_option(name) end end + return result end diff --git a/spec/shared_behaviours/things_that_declare_options.rb b/spec/shared_behaviours/things_that_declare_options.rb index 5300a159a..4ffe55d07 100755 --- a/spec/shared_behaviours/things_that_declare_options.rb +++ b/spec/shared_behaviours/things_that_declare_options.rb @@ -37,9 +37,12 @@ shared_examples_for "things that declare options" do it "should list all the options" do thing = add_options_to do option "--foo" - option "--bar" + option "--bar", '-b' + option "-q", "--quux" + option "-f" + option "--baz" end - thing.options.should =~ [:foo, :bar] + thing.options.should == [:foo, :bar, :b, :q, :quux, :f, :baz] end it "should detect conflicts in long options" do diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb index 23d0de490..8d0606d04 100755 --- a/spec/unit/interface/action_spec.rb +++ b/spec/unit/interface/action_spec.rb @@ -249,180 +249,247 @@ describe Puppet::Interface::Action do end end - context "with action decorators" do - context "local only" do + context "with decorators" do + context "declared locally" do let :face do Puppet::Interface.new(:action_decorators, '0.0.1') do action :bar do when_invoked do true end end - def report(arg) end + def reported; @reported; end + def report(arg) + (@reported ||= []) << arg + end end end - it "should call action before decorators" do - face.action(:baz) do - option "--baz" do - before_action do |action, args, options| - report(:action_option) - end - end - when_invoked do true end + it "should execute before advice on action options in declaration order" do + face.action(:boo) do + option("--foo") { before_action { |_,_,_| report :foo } } + option("--bar", '-b') { before_action { |_,_,_| report :bar } } + option("-q", "--quux") { before_action { |_,_,_| report :quux } } + option("-f") { before_action { |_,_,_| report :f } } + option("--baz") { before_action { |_,_,_| report :baz } } + when_invoked { } end - face.expects(:report).with(:action_option) - face.baz :baz => true + face.boo :foo => 1, :bar => 1, :quux => 1, :f => 1, :baz => 1 + face.reported.should == [ :foo, :bar, :quux, :f, :baz ] end - it "should call action after decorators" do - face.action(:baz) do - option "--baz" do - after_action do |action, args, options| - report(:action_option) - end - end - when_invoked do true end + it "should execute after advice on action options in declaration order" do + face.action(:boo) do + option("--foo") { after_action { |_,_,_| report :foo } } + option("--bar", '-b') { after_action { |_,_,_| report :bar } } + option("-q", "--quux") { after_action { |_,_,_| report :quux } } + option("-f") { after_action { |_,_,_| report :f } } + option("--baz") { after_action { |_,_,_| report :baz } } + when_invoked { } end - face.expects(:report).with(:action_option) - face.baz :baz => true + face.boo :foo => 1, :bar => 1, :quux => 1, :f => 1, :baz => 1 + face.reported.should == [ :foo, :bar, :quux, :f, :baz ].reverse end - it "should call local before decorators" do - face.option "--foo FOO" do - before_action do |action, args, options| - report(:before) - end + it "should execute before advice on face options in declaration order" do + face.instance_eval do + option("--foo") { before_action { |_,_,_| report :foo } } + option("--bar", '-b') { before_action { |_,_,_| report :bar } } + option("-q", "--quux") { before_action { |_,_,_| report :quux } } + option("-f") { before_action { |_,_,_| report :f } } + option("--baz") { before_action { |_,_,_| report :baz } } end - face.expects(:report).with(:before) - face.bar({:foo => 12}) + face.script(:boo) { } + + face.boo :foo => 1, :bar => 1, :quux => 1, :f => 1, :baz => 1 + face.reported.should == [ :foo, :bar, :quux, :f, :baz ] end - it "should call local after decorators" do - face.option "--foo FOO" do - after_action do |action, args, options| report(:after) end + it "should execute after advice on face options in declaration order" do + face.instance_eval do + option("--foo") { after_action { |_,_,_| report :foo } } + option("--bar", '-b') { after_action { |_,_,_| report :bar } } + option("-q", "--quux") { after_action { |_,_,_| report :quux } } + option("-f") { after_action { |_,_,_| report :f } } + option("--baz") { after_action { |_,_,_| report :baz } } end - face.expects(:report).with(:after) - face.bar({:foo => 12}) + face.script(:boo) { } + + face.boo :foo => 1, :bar => 1, :quux => 1, :f => 1, :baz => 1 + face.reported.should == [ :foo, :bar, :quux, :f, :baz ].reverse end - context "with inactive decorators" do - it "should not invoke a decorator if the options are empty" do - face.option "--foo FOO" do - before_action do |action, args, options| - report :before_action - end + it "should execute before advice on face options before action options" do + face.instance_eval do + option("--face-foo") { before_action { |_,_,_| report :face_foo } } + option("--face-bar", '-r') { before_action { |_,_,_| report :face_bar } } + action(:boo) do + option("--action-foo") { before_action { |_,_,_| report :action_foo } } + option("--action-bar", '-b') { before_action { |_,_,_| report :action_bar } } + option("-q", "--action-quux") { before_action { |_,_,_| report :action_quux } } + option("-a") { before_action { |_,_,_| report :a } } + option("--action-baz") { before_action { |_,_,_| report :action_baz } } + when_invoked { } + end + option("-u", "--face-quux") { before_action { |_,_,_| report :face_quux } } + option("-f") { before_action { |_,_,_| report :f } } + option("--face-baz") { before_action { |_,_,_| report :face_baz } } + end + + expected_calls = [ :face_foo, :face_bar, :face_quux, :f, :face_baz, + :action_foo, :action_bar, :action_quux, :a, :action_baz ] + face.boo Hash[ *expected_calls.zip([]).flatten ] + face.reported.should == expected_calls + end + + it "should execute after advice on face options in declaration order" do + face.instance_eval do + option("--face-foo") { after_action { |_,_,_| report :face_foo } } + option("--face-bar", '-r') { after_action { |_,_,_| report :face_bar } } + action(:boo) do + option("--action-foo") { after_action { |_,_,_| report :action_foo } } + option("--action-bar", '-b') { after_action { |_,_,_| report :action_bar } } + option("-q", "--action-quux") { after_action { |_,_,_| report :action_quux } } + option("-a") { after_action { |_,_,_| report :a } } + option("--action-baz") { after_action { |_,_,_| report :action_baz } } + when_invoked { |options| warn options.inspect } end - face.expects(:report).never # I am testing the negative. - face.bar + option("-u", "--face-quux") { after_action { |_,_,_| report :face_quux } } + option("-f") { after_action { |_,_,_| report :f } } + option("--face-baz") { after_action { |_,_,_| report :face_baz } } end - context "with some decorators only" do - before :each do - face.option "--foo" do - before_action do |action, args, options| report :foo end - end - face.option "--bar" do - before_action do |action, args, options| report :bar end - end - end + expected_calls = [ :face_foo, :face_bar, :face_quux, :f, :face_baz, + :action_foo, :action_bar, :action_quux, :a, :action_baz ] + face.boo Hash[ *expected_calls.zip([]).flatten ] + face.reported.should == expected_calls.reverse + end - it "should work with the foo option" do - face.expects(:report).with(:foo) - face.bar(:foo => true) - end + it "should not invoke a decorator if the options are empty" do + face.option("--foo FOO") { before_action { |_,_,_| report :before_action } } + face.expects(:report).never + face.bar + end - it "should work with the bar option" do - face.expects(:report).with(:bar) - face.bar(:bar => true) - end + context "passing a subset of the options" do + before :each do + face.option("--foo") { before_action { |_,_,_| report :foo } } + face.option("--bar") { before_action { |_,_,_| report :bar } } + end - it "should work with both options" do - face.expects(:report).with(:foo) - face.expects(:report).with(:bar) - face.bar(:foo => true, :bar => true) - end + it "should invoke only foo's advice when passed only 'foo'" do + face.bar(:foo => true) + face.reported.should == [ :foo ] + end + + it "should invoke only bar's advice when passed only 'bar'" do + face.bar(:bar => true) + face.reported.should == [ :bar ] + end + + it "should invoke advice for all passed options" do + face.bar(:foo => true, :bar => true) + face.reported.should == [ :foo, :bar ] end end end - context "with inherited decorators" do + context "and inheritance" do let :parent do - parent = Class.new(Puppet::Interface) - parent.script :on_parent do :on_parent end - parent.define_method :report do |arg| arg end - parent + Class.new(Puppet::Interface) do + script(:on_parent) { :on_parent } + + def reported; @reported; end + def report(arg) + (@reported ||= []) << arg + end + end end let :child do - child = parent.new(:inherited_decorators, '0.0.1') do - script :on_child do :on_child end + parent.new(:inherited_decorators, '0.0.1') do + script(:on_child) { :on_child } end end - context "with a child decorator" do + context "locally declared face options" do subject do - child.option "--foo FOO" do - before_action do |action, args, options| - report(:child_before) - end - end - child.expects(:report).with(:child_before) + child.option("--foo=") { before_action { |_,_,_| report :child_before } } child end - it "child actions should invoke the decorator" do - subject.on_child({:foo => true, :bar => true}).should == :on_child + it "should be invoked when calling a child action" do + subject.on_child(:foo => true, :bar => true).should == :on_child + subject.reported.should == [ :child_before ] end - it "parent actions should invoke the decorator" do - subject.on_parent({:foo => true, :bar => true}).should == :on_parent + it "should be invoked when calling a parent action" do + subject.on_parent(:foo => true, :bar => true).should == :on_parent + subject.reported.should == [ :child_before ] end end - context "with a parent decorator" do + context "inherited face option decorators" do subject do - parent.option "--foo FOO" do - before_action do |action, args, options| - report(:parent_before) - end - end - child.expects(:report).with(:parent_before) + parent.option("--foo=") { before_action { |_,_,_| report :parent_before } } child end - it "child actions should invoke the decorator" do - subject.on_child({:foo => true, :bar => true}).should == :on_child + it "should be invoked when calling a child action" do + subject.on_child(:foo => true, :bar => true).should == :on_child + subject.reported.should == [ :parent_before ] end - it "parent actions should invoke the decorator" do - subject.on_parent({:foo => true, :bar => true}).should == :on_parent + it "should be invoked when calling a parent action" do + subject.on_parent(:foo => true, :bar => true).should == :on_parent + subject.reported.should == [ :parent_before ] end end - context "with child and parent decorators" do + context "with both inherited and local face options" do + # Decorations should be invoked in declaration order, according to + # inheritance (e.g. parent class options should be handled before + # subclass options). subject do - parent.option "--foo FOO" do - before_action { |action, args, options| report(:parent_before) } - after_action { |action, args, options| report(:parent_after) } + child.option "-c" do + before_action { |action, args, options| report :c_before } + after_action { |action, args, options| report :c_after } end - child.option "--bar BAR" do - before_action { |action, args, options| report(:child_before) } - after_action { |action, args, options| report(:child_after) } + + parent.option "-a" do + before_action { |action, args, options| report :a_before } + after_action { |action, args, options| report :a_after } + end + + child.option "-d" do + before_action { |action, args, options| report :d_before } + after_action { |action, args, options| report :d_after } + end + + parent.option "-b" do + before_action { |action, args, options| report :b_before } + after_action { |action, args, options| report :b_after } end - child.expects(:report).with(:child_before) - child.expects(:report).with(:parent_before) - child.expects(:report).with(:parent_after) - child.expects(:report).with(:child_after) + child.script(:decorations) { report :invoked } child end - it "child actions should invoke all the decorator" do - subject.on_child({:foo => true, :bar => true}).should == :on_child + it "should invoke all decorations when calling a child action" do + subject.decorations(:a => 1, :b => 1, :c => 1, :d => 1) + subject.reported.should == [ + :a_before, :b_before, :c_before, :d_before, + :invoked, + :d_after, :c_after, :b_after, :a_after + ] end - it "parent actions should invoke all the decorator" do - subject.on_parent({:foo => true, :bar => true}).should == :on_parent + it "should invoke all decorations when calling a parent action" do + subject.decorations(:a => 1, :b => 1, :c => 1, :d => 1) + subject.reported.should == [ + :a_before, :b_before, :c_before, :d_before, + :invoked, + :d_after, :c_after, :b_after, :a_after + ] end end end |