[PATCH] (#7282) action without `when_invoked` should fail...
Daniel Pittman <daniel <at> puppetlabs.com>
2011-05-02 17:19:28 GMT
We used to let actions be declared without the `when_invoked` block, which was
usually a sign of either someone writing their method code direct in action
declaration, or someone forgetting to add their code at all.
This was just let silently by: the error only showed up when you finally tried
to invoke the action, and a NoMethod error was raised by the face.
...except for our own testing. We took advantage of this a whole pile of
times in there; fixing the original UI issue means fixing all those too.
Reviewed-By: Nick Lewis <nick <at> puppetlabs.com>
---
lib/puppet/interface/action.rb | 5 +++-
lib/puppet/interface/action_builder.rb | 1 +
spec/lib/puppet/face/huzzah.rb | 2 +-
spec/unit/interface/action_builder_spec.rb | 37 +++++++++++++++++++++++++---
spec/unit/interface/action_manager_spec.rb | 16 +++++++----
spec/unit/interface/action_spec.rb | 24 +++++++++++++----
spec/unit/interface_spec.rb | 14 +++++++----
7 files changed, 76 insertions(+), 23 deletions(-)
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 0dbdd57..203d808 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
<at> <at> -171,7 +171,8 <at> <at> class Puppet::Interface::Action
# this stuff work, because it would have been cleaner. Which gives you an
# idea how motivated we were to make this cleaner. Sorry.
# --daniel 2011-03-31
- attr_reader :positional_arg_count
+ attr_reader :positional_arg_count
+ attr_accessor :when_invoked
def when_invoked=(block)
internal_name = "#{ <at> name} implementation, required on Ruby 1.8".to_sym
<at> <at> -219,9 +220,11 <at> <at> WRAPPER
if <at> face.is_a?(Class)
<at> face.class_eval do eval wrapper, nil, file, line end
<at> face.define_method(internal_name, &block)
+ <at> when_invoked = <at> face.instance_method(name)
else
<at> face.instance_eval do eval wrapper, nil, file, line end
<at> face.meta_def(internal_name, &block)
+ <at> when_invoked = <at> face.method(name).unbind
end
end
diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb
index afc49e9..0bf4f14 100644
--- a/lib/puppet/interface/action_builder.rb
+++ b/lib/puppet/interface/action_builder.rb
<at> <at> -69,5 +69,6 <at> <at> class Puppet::Interface::ActionBuilder
<at> face = face
<at> action = Puppet::Interface::Action.new(face, name)
instance_eval(&block)
+ <at> action.when_invoked or raise ArgumentError, "actions need to know what to do when_invoked; please add
the block"
end
end
diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb
index 2c2b7aa..6593d35 100755
--- a/spec/lib/puppet/face/huzzah.rb
+++ b/spec/lib/puppet/face/huzzah.rb
<at> <at> -3,5 +3,5 <at> <at> Puppet::Face.define(:huzzah, '2.0.1') do
copyright "Puppet Labs", 2011
license "Apache 2 license; see COPYING"
summary "life is a thing for celebration"
- action :bar do "is where beer comes from" end
+ script :bar do "is where beer comes from" end
end
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index bf7afa7..e0d0ebe 100755
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
<at> <at> -7,7 +7,8 <at> <at> describe Puppet::Interface::ActionBuilder do
let :face do Puppet::Interface.new(:puppet_interface_actionbuilder, '0.0.1') end
it "should build an action" do
- action = Puppet::Interface::ActionBuilder.build(nil, :foo) do
+ action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
end
action.should be_a(Puppet::Interface::Action)
action.name.should == :foo
<at> <at> -26,17 +27,25 <at> <at> describe Puppet::Interface::ActionBuilder do
should raise_error("Action :foo must specify a block")
end
+ it "should require an invocation block" do
+ expect {
+ Puppet::Interface::ActionBuilder.build(face, :foo) {}
+ }.to raise_error(/actions need to know what to do when_invoked; please add the block/)
+ end
+
describe "when handling options" do
it "should have a #option DSL function" do
method = nil
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
method = self.method(:option)
end
- method.should be
+ method.should be_an_instance_of Method
end
it "should define an option without a block" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
option "--bar"
end
action.should be_option :bar
<at> <at> -44,6 +53,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should accept an empty block" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
option "--bar" do
# This space left deliberately blank.
end
<at> <at> -55,6 +65,7 <at> <at> describe Puppet::Interface::ActionBuilder do
context "inline documentation" do
it "should set the summary" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
summary "this is some text"
end
action.summary.should == "this is some text"
<at> <at> -64,13 +75,16 <at> <at> describe Puppet::Interface::ActionBuilder do
context "action defaulting" do
it "should set the default to true" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
default
end
action.default.should be_true
end
it "should not be default by, er, default. *cough*" do
- action = Puppet::Interface::ActionBuilder.build(face, :foo) do end
+ action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
+ end
action.default.should be_false
end
end
<at> <at> -79,6 +93,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if no rendering format is given" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering do true end
end
}.to raise_error ArgumentError, /must give a rendering format to when_rendering/
<at> <at> -87,6 +102,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if no block is given" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json
end
}.to raise_error ArgumentError, /must give a block to when_rendering/
<at> <at> -95,6 +111,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if the block takes no arguments" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do true end
end
}.to raise_error ArgumentError, /when_rendering methods take one argument, the result, not/
<at> <at> -103,6 +120,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if the block takes more than one argument" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |a, b, c| true end
end
}.to raise_error ArgumentError, /when_rendering methods take one argument, the result, not/
<at> <at> -111,6 +129,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if the block takes a variable number of arguments" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |*args| true end
end
}.to raise_error(ArgumentError,
<at> <at> -119,6 +138,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should stash a rendering block" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |a| true end
end
action.when_rendering(:json).should be_an_instance_of Method
<at> <at> -127,6 +147,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if you try to set the same rendering twice" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |a| true end
when_rendering :json do |a| true end
end
<at> <at> -135,6 +156,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should work if you set two different renderings" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |a| true end
when_rendering :yaml do |a| true end
end
<at> <at> -144,6 +166,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should be bound to the face when called" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
when_rendering :json do |a| self end
end
action.when_rendering(:json).call(true).should == face
<at> <at> -152,13 +175,16 <at> <at> describe Puppet::Interface::ActionBuilder do
context "#render_as" do
it "should default to nil (eg: based on context)" do
- action = Puppet::Interface::ActionBuilder.build(face, :foo) do end
+ action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
+ end
action.render_as.should be_nil
end
it "should fail if not rendering format is given" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
render_as
end
}.to raise_error ArgumentError, /must give a rendering format to render_as/
<at> <at> -167,6 +193,7 <at> <at> describe Puppet::Interface::ActionBuilder do
Puppet::Network::FormatHandler.formats.each do |name|
it "should accept #{name.inspect} format" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
render_as name
end
action.render_as.should == name
<at> <at> -175,6 +202,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should accept :for_humans format" do
action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
render_as :for_humans
end
action.render_as.should == :for_humans
<at> <at> -184,6 +212,7 <at> <at> describe Puppet::Interface::ActionBuilder do
it "should fail if given #{input.inspect}" do
expect {
Puppet::Interface::ActionBuilder.build(face, :foo) do
+ when_invoked do true end
render_as input
end
}.to raise_error ArgumentError, /#{input.inspect} is not a valid rendering format/
diff --git a/spec/unit/interface/action_manager_spec.rb b/spec/unit/interface/action_manager_spec.rb
index 07d517c..5a479ad 100755
--- a/spec/unit/interface/action_manager_spec.rb
+++ b/spec/unit/interface/action_manager_spec.rb
<at> <at> -94,7 +94,7 <at> <at> describe Puppet::Interface::ActionManager do
end
it "should be able to indicate when an action is defined" do
- subject.action(:foo) { "something" }
+ subject.action(:foo) { when_invoked do true end }
subject.should be_action(:foo)
end
end
<at> <at> -218,25 +218,29 <at> <at> describe Puppet::Interface::ActionManager do
describe "#action" do
it 'should add an action' do
- subject.action(:foo) { }
+ subject.action(:foo) { when_invoked do true end }
subject.get_action(:foo).should be_a Puppet::Interface::Action
end
it 'should support default actions' do
- subject.action(:foo) { default }
+ subject.action(:foo) { when_invoked do true end; default }
subject.get_default_action.should == subject.get_action(:foo)
end
it 'should not support more than one default action' do
- subject.action(:foo) { default }
- expect { subject.action(:bar) { default } }.should raise_error
+ subject.action(:foo) { when_invoked do true end; default }
+ expect { subject.action(:bar) {
+ when_invoked do true end
+ default
+ }
+ }.should raise_error /cannot both be default/
end
end
describe "#get_action" do
let :parent_class do
parent_class = Class.new(Puppet::Interface)
- parent_class.action(:foo) {}
+ parent_class.action(:foo) { when_invoked do true end }
parent_class
end
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index f8102f4..e799609 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
<at> <at> -150,6 +150,7 <at> <at> describe Puppet::Interface::Action do
it "should support options with an empty block" do
face = Puppet::Interface.new(:action_level_options, '0.0.1') do
action :foo do
+ when_invoked do true end
option "--bar" do
# this line left deliberately blank
end
<at> <at> -162,7 +163,10 <at> <at> describe Puppet::Interface::Action do
it "should return only action level options when there are no face options" do
face = Puppet::Interface.new(:action_level_options, '0.0.1') do
- action :foo do option "--bar" end
+ action :foo do
+ when_invoked do true end
+ option "--bar"
+ end
end
face.get_action(:foo).options.should =~ [:bar]
<at> <at> -171,8 +175,8 <at> <at> describe Puppet::Interface::Action do
describe "with both face and action options" do
let :face do
Puppet::Interface.new(:action_level_options, '0.0.1') do
- action :foo do option "--bar" end
- action :baz do option "--bim" end
+ action :foo do when_invoked do true end ; option "--bar" end
+ action :baz do when_invoked do true end ; option "--bim" end
option "--quux"
end
end
<at> <at> -186,7 +190,10 <at> <at> describe Puppet::Interface::Action do
parent.option "--foo"
child = parent.new(:inherited_options, '0.0.1') do
option "--bar"
- action :action do option "--baz" end
+ action :action do
+ when_invoked do true end
+ option "--baz"
+ end
end
action = child.get_action(:action)
<at> <at> -215,7 +222,10 <at> <at> describe Puppet::Interface::Action do
it_should_behave_like "things that declare options" do
def add_options_to(&block)
face = Puppet::Interface.new(:with_options, '0.0.1') do
- action(:foo, &block)
+ action(:foo) do
+ when_invoked do true end
+ self.instance_eval &block
+ end
end
face.get_action(:foo)
end
<at> <at> -498,7 +508,9 <at> <at> describe Puppet::Interface::Action do
it_should_behave_like "documentation on faces" do
subject do
face = Puppet::Interface.new(:action_documentation, '0.0.1') do
- action :documentation do end
+ action :documentation do
+ when_invoked do true end
+ end
end
face.get_action(:documentation)
end
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 27da397..e28e55a 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
<at> <at> -146,6 +146,7 <at> <at> describe Puppet::Interface do
option "--foo"
option "--bar"
action :baz do
+ when_invoked { true }
option "--quux"
end
end
<at> <at> -155,7 +156,10 <at> <at> describe Puppet::Interface do
it "should fail when a face option duplicates an action option" do
expect {
subject.new(:action_level_options, '0.0.1') do
- action :bar do option "--foo" end
+ action :bar do
+ when_invoked { true }
+ option "--foo"
+ end
option "--foo"
end
}.should raise_error ArgumentError, /Option foo conflicts with existing option foo on/i
<at> <at> -163,8 +167,8 <at> <at> describe Puppet::Interface do
it "should work when two actions have the same option" do
face = subject.new(:with_options, '0.0.1') do
- action :foo do option "--quux" end
- action :bar do option "--quux" end
+ action :foo do when_invoked { true } ; option "--quux" end
+ action :bar do when_invoked { true } ; option "--quux" end
end
face.get_action(:foo).options.should =~ [:quux]
<at> <at> -176,14 +180,14 <at> <at> describe Puppet::Interface do
let :parent do
parent = Class.new(subject)
parent.option("--inherited")
- parent.action(:parent_action) do end
+ parent.action(:parent_action) do when_invoked { true } end
parent
end
let :face do
face = parent.new(:example, '0.2.1')
face.option("--local")
- face.action(:face_action) do end
+ face.action(:face_action) do when_invoked { true } end
face
end
--
1.7.5
--
--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev <at> googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
RSS Feed