I think that is a terrible design, and should be backed out. Specifically, it is a degenerate form of multiple inheritance where *part* of the API of another face is imported into the current face. This is ... well, confusing would be the polite way to put it.
This is especially true because you have internally filtered the list of inherited options to avoid conflicts, leading to a mechanism that will "inherit everything, except where it doesn't." I can't think of any pattern from regular OO design that mirrors this, which is always a bad sign: the closest mirrors are delegation patterns, but this only adopts *part* of the delegated object, not all. I can't think of a sane use case that doesn't lead to confusion; in the cloud case: If you are adding absolutely *zero* behaviour to the underlying faces then this might, maybe, be stretched to make sense. If not... Well, an example: one of the reasons for having the wrapper is to extract data from the first operation about the target machine address and login details. What should you do if those are given on the command line as well? I can think of no coherent behaviour around that: either you ignore deliberately given information from the user, or just override the internal details you deliberately obtained. Neither is good, in API design, or CLI design. You seem to have taken no account of option hooks in this code, and certainly have no tests. I do not believe you can reliably, without substantially more change, inherit those; this is another "inherit the form, but not the function" part of the design. Finally, a much, much, *much* saner way to achieve the same result would be to do simple meta-programming: at the point you want to adopt options from another face, either hand-write them, or open-coding the same structure. I don't think this needs language and API support, even if it is a genuine need. Which, frankly, I don't believe. Daniel On Mon, Apr 25, 2011 at 12:48, Pieter van de Bruggen <[email protected]> wrote: > In many cases when you're composing actions (but not every one), you'll want > the composed action to take the options for the actions it wraps. This > includes not only option names, but documentation and validation routines as > well. > Especially in cases where the options are distinct sets, because the > composed actions are sequential transformations, this will be very useful; > I've implemented it in support of CloudPack, which demonstrates several such > cases. > > On Mon, Apr 25, 2011 at 12:23 PM, Luke Kanies <[email protected]> wrote: >> >> Can you give an example of how this would be used in real life? >> >> -- >> http://puppetlabs.com/ | +1-615-594-8199 | @puppetmasterd >> >> On Apr 25, 2011, at 8:20 PM, Pieter van de Bruggen >> <[email protected]> wrote: >> >> > Reviewed-By: Matt Robinson >> > >> > Signed-off-by: Pieter van de Bruggen <[email protected]> >> > --- >> > Local-branch: tickets/2.7.next/7220 >> > lib/puppet/interface/action.rb | 5 +++ >> > lib/puppet/interface/action_builder.rb | 10 +++++ >> > spec/unit/interface/action_builder_spec.rb | 55 >> > ++++++++++++++++++++++++++++ >> > spec/unit/interface/action_spec.rb | 55 >> > ++++++++++++++++++++++++++++ >> > 4 files changed, 125 insertions(+), 0 deletions(-) >> > >> > diff --git a/lib/puppet/interface/action.rb >> > b/lib/puppet/interface/action.rb >> > index 08bc0a3..f8eef69 100644 >> > --- a/lib/puppet/interface/action.rb >> > +++ b/lib/puppet/interface/action.rb >> > @@ -217,6 +217,11 @@ WRAPPER >> > option >> > end >> > >> > + def inherit_options_from(action) >> > + options = action.options.map { |opt| action.get_option(opt, false) >> > } >> > + options.reject!(&:nil?).uniq.each { |option| add_option(option) } >> > + end >> > + >> > def option?(name) >> > @options.include? name.to_sym >> > end >> > diff --git a/lib/puppet/interface/action_builder.rb >> > b/lib/puppet/interface/action_builder.rb >> > index 2ffa387..fd8b085 100644 >> > --- a/lib/puppet/interface/action_builder.rb >> > +++ b/lib/puppet/interface/action_builder.rb >> > @@ -38,6 +38,16 @@ class Puppet::Interface::ActionBuilder >> > @action.add_option(option) >> > end >> > >> > + def inherit_options_from(action) >> > + if action.is_a? Symbol >> > + name = action >> > + action = @face.get_action(name) or >> > + raise ArgumentError, "This Face has no action named #{name}!" >> > + end >> > + >> > + @action.inherit_options_from(action) >> > + end >> > + >> > def default(value = true) >> > @action.default = !!value >> > end >> > diff --git a/spec/unit/interface/action_builder_spec.rb >> > b/spec/unit/interface/action_builder_spec.rb >> > index bf7afa7..6f36d2b 100755 >> > --- a/spec/unit/interface/action_builder_spec.rb >> > +++ b/spec/unit/interface/action_builder_spec.rb >> > @@ -52,6 +52,61 @@ describe Puppet::Interface::ActionBuilder do >> > end >> > end >> > >> > + describe "#inherit_options_from" do >> > + let :face do >> > + Puppet::Interface.new(:face_with_some_options, '0.0.1') do >> > + option '-w' >> > + >> > + action(:foo) do >> > + option '-x', '--ex' >> > + option '-y', '--why' >> > + end >> > + >> > + action(:bar) do >> > + option '-z', '--zee' >> > + end >> > + >> > + action(:baz) do >> > + option '-z', '--zed' >> > + end >> > + end >> > + end >> > + >> > + it 'should add the options from the specified action' do >> > + foo = face.get_action(:foo) >> > + action = Puppet::Interface::ActionBuilder.build(face, >> > :inherit_options) do >> > + inherit_options_from foo >> > + end >> > + action.options.should == foo.options >> > + end >> > + >> > + it 'should add the options from multiple actions' do >> > + foo = face.get_action(:foo) >> > + bar = face.get_action(:bar) >> > + action = Puppet::Interface::ActionBuilder.build(face, >> > :inherit_options) do >> > + inherit_options_from foo >> > + inherit_options_from bar >> > + end >> > + action.options.should == (foo.options + bar.options).uniq.sort >> > + end >> > + >> > + it 'should permit symbolic names for actions in the same face' do >> > + foo = face.get_action(:foo) >> > + action = Puppet::Interface::ActionBuilder.build(face, >> > :inherit_options) do >> > + inherit_options_from :foo >> > + end >> > + action.options.should == foo.options >> > + end >> > + >> > + it 'should raise a useful error if you supply a bad action name' do >> > + expect do >> > + Puppet::Interface::ActionBuilder.build(face, :inherit_options) >> > do >> > + inherit_options_from :nowhere >> > + end >> > + end.to raise_error /nowhere/ >> > + end >> > + end >> > + >> > context "inline documentation" do >> > it "should set the summary" do >> > action = Puppet::Interface::ActionBuilder.build(face, :foo) do >> > diff --git a/spec/unit/interface/action_spec.rb >> > b/spec/unit/interface/action_spec.rb >> > index 24826a6..735fbcb 100755 >> > --- a/spec/unit/interface/action_spec.rb >> > +++ b/spec/unit/interface/action_spec.rb >> > @@ -68,6 +68,61 @@ describe Puppet::Interface::Action do >> > end >> > end >> > >> > + describe "#inherit_options_from" do >> > + let :face do >> > + Puppet::Interface.new(:face_with_some_options, '0.0.1') do >> > + option '-w' >> > + >> > + action(:foo) do >> > + option '-x', '--ex' >> > + option '-y', '--why' >> > + end >> > + >> > + action(:bar) do >> > + option '-z', '--zee' >> > + end >> > + >> > + action(:baz) do >> > + option '-z', '--zed' >> > + end >> > + >> > + action(:noopts) do >> > + # no options declared >> > + end >> > + end >> > + end >> > + >> > + subject { action = face.action(:new_action) { } } >> > + >> > + it 'should add the options from the specified action' do >> > + subject.inherit_options_from(foo = face.get_action(:foo)) >> > + subject.options.should == foo.options >> > + end >> > + >> > + it 'should not die when the specified action has no options' do >> > + original_options = subject.options >> > + subject.inherit_options_from(face.get_action(:noopts)) >> > + subject.options.should == original_options >> > + end >> > + >> > + it 'should add the options from multiple actions' do >> > + subject.inherit_options_from(foo = face.get_action(:foo)) >> > + subject.inherit_options_from(bar = face.get_action(:bar)) >> > + subject.options.should == (foo.options + bar.options).uniq.sort >> > + end >> > + >> > + it 'should not inherit face options' do >> > + subject.expects(:add_option) >> > + subject.expects(:add_option).with(face.get_option(:w)).never >> > + subject.inherit_options_from(face.get_action(:bar)) >> > + end >> > + >> > + it 'should raise an error if inheritance would duplicate options' >> > do >> > + subject.inherit_options_from(face.get_action(:bar)) >> > + expect { subject.inherit_options_from(face.get_action(:baz)) }.to >> > raise_error >> > + end >> > + end >> > + >> > describe "when invoking" do >> > it "should be able to call other actions on the same object" do >> > face = Puppet::Interface.new(:my_face, '0.0.1') do >> > -- >> > 1.7.4.1 >> > >> > -- >> > 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. >> > >> >> -- >> 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. >> > > -- > 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. > -- ⎋ Puppet Labs Developer – http://puppetlabs.com ✉ Daniel Pittman <[email protected]> ✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775 ♲ Made with 100 percent post-consumer electrons -- 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.
