On Fri, Sep 13, 2013 at 9:10 AM, Andy Parker <[email protected]> wrote:
> More comments inline, but I'm not wedded to the _hook suggestion. Thoughts > from others? > > I think this really turns into a question (right now, at least) of > consistency in the provider interface versus trying to call out methods as > "hooks". > > On Thu, Sep 12, 2013 at 8:39 PM, badgerious <[email protected]> wrote: > >> I sort of dislike appending ‘_hook’, because: >> >> 1. It’s not consistent with other methods. I think it loses some of its >> usefulness (i.e. make it easier to identify hook methods) if only a few >> hook methods actually use the convention. The remedy for this seems to be >> renaming all existing hook methods, but this seems like a lot of pain for a >> small gain. >> > > I was one of the +1s that Adrien referenced, but I also share your > concern. I think if we could get everything as _hook it would be great, but > that would take identifying everything we have, updating docs, and keeping > backwards compat shims in place. > My motivation for this was from talking with Tom Linkin when adding a hook for parameter validation at the end of catalog compilation. Right now Puppet::Resource#finish is called, which is a completely opaque name (especially when we also have `#flush` and #finalize` methods defined elsewhere.) I decided we should go with a method called `#post_compile_hook` because it was a start in the right direction. In the medium - long term I think it would be very important to do a comprehensive sweep and rename hook methods/callbacks/potential extension points to give them more descriptive names, document them, add shims where methods were renamed, et cetera. However in the very short term I've been occupied with catching up on backlogged pull requests, so I haven't had the time to push this forward. I do think this is important though. > > 2. It’s only sort of helpful; it tells you to expect that something >> external to the current class/module will call this method, but what, when, >> and why? It’s still ultimately a magic method which you’ll need to look up >> to figure out what it does. >> > > That would be alleviated if we could get some better docs in place that > outline all of the methods available for implementation on the provider. > > >> Here are some alternative suggestions: >> >> A. Continue with the current naming scheme (i.e. just a descriptive name, >> no ‘_hook’), and focus on documenting hooks in a friendly format. For >> example, it would be nice to have a clear picture of a provider’s >> “interface” in the provider development docs (I googled yard docs and found >> some, but they were only sort of helpful. It seems like, for example, the >> 'abstract' methods such as 'prefetch' are missing from the provider doc >> page; not sure what's going on there). This avoids the need to rename a >> bunch of stuff (or have inconsistent names), which would probably take >> quite a while and be painful. >> > > We've been trying to reverse-engineer the exact interface. The yard docs > were our first attempt at that, and we knew that they weren't complete. > Having some help on fleshing them out would be great. > Right now a number of our method names are... bad. For instance, #flush vs #finalize vs #finish and all that. I was of the opinion that on top of documenting them, we should give them names that call out the context in which they were called as well. > > > B. (more radical) Allow hook methods to be implemented as a method with >> any name but require them to be “registered”, e.g.: >> >> def my_method_name(resources) >> … >> end >> register_hook :prefetch, :my_method_name >> >> or maybe something more explicit (to address 2 from above) like: >> >> Puppet::Transaction.register_hook self, :prefetch, :my_method_name >> >> or maybe >> >> Puppet::Transaction.prefetch_hook(self) do >> … >> end >> >> This would also prevent accidentally implementing a magic method >> (something that might be a concern in the PR I mentioned up top). A change >> like this would probably be more trouble than it's worth, but just a >> thought. >> >> >> > I am all for this kind of explicit style, but it is an even larger > departure from what we have. > I've played around with patterns like this in my own projects, and there are pros and cons. You can get more flexibility and power when declaring hooks, but it makes things much more complex. If someone was willing to spend some brain time to define a good interface then it might be worth it, but in a lot of cases `obj.invoke_method if obj.respond_to? :invoke_method` is all you need. -- Adrien Thebo | Puppet Labs -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/puppet-dev. For more options, visit https://groups.google.com/groups/opt_out.
