On 06/10/10 15:57 -0400, Toby Crawley wrote:
The issue is that all three of these methods are defined on the class as
closures over the action list for a particular object. So if you are
iterating over a collection, *all* members of the collection will have
the three methods defined for the *last* collection member, since the
methods are defined at the class level. This results in:
irb> client.keys.collect {|x| [x.id, x.actions_urls]}
[
[0] [
[0] "somekey",
[1] {
"destroy" =>
"http://localhost:8080/deltacloud/api/keys/default";
}
],
[1] [
[0] "default",
[1] {
"destroy" =>
"http://localhost:8080/deltacloud/api/keys/default";
}
]
]
which means a call to client.keys.first.destroy! actually destroys
client.keys.last.
xml_to_class has a good bit of meta-programming, and is difficult to
decipher. Is there a reason why the client starts with a bare Class, and
turns it in to Instance, Key, etc? I understand the motivation of taking
some base class and creating Instance, Key, etc. dynamically, but
wouldn't it make sense to have a base class that provided some
functionality (written off the cuff, and untested):
class DeltaCloud::API::BaseObject
def action_meta
@action_meta ||= { }
end
def add_action(action, method, url)
action_meta[action.to_s] = [method, url]
# you could also define_method here instead of relying on
method_missing (below)
end
def actions
actions_meta.keys
end
def actions_urls
action_meta.inject({}) { |accum, action, method_and_url|
accum[action] = method_and_url.last; accum }
end
def call_action(action)
method, url = action_meta[action]
client.request(method.to_sym, url, {}, {})
# would need to get status here
end
def method_missing(meth, *args, &block)
if meth.to_s =~ /^(.*)!$/ and actions.include?($1)
call_action($1)
else
super
end
end
end
Yes, that's true. I know that xml_to_class method is monstrous and it can
cause serious troubles like action_url issue. I'm totally open for
splitting this method to smallest chunks, which will be more maintainable
and at least readable.
I like idea of having:
'BaseObject' (For holding 'reals', 'hardware_profiles' and stuff like this)
|
-> 'ActionObject' ('instances', 'buckets', 'volumes', 'keys')
So 'BaseObject' should hold all resource which is some kind of 'static',
means they doesn't have .action! methods. And 'ActionObject', which will
add 'actions'. Let me know what do you think.
DeltaCloud.define_class could then instead create a new class that
subclassed the above class, and xml_to_class (for actions at least)
becomes much simpler:
when "actions":
actions = []
attribute.xpath('link').each do |link|
actions << [link['rel'], link[:href]]
define_method :"#{link['rel'].sanitize}!" do
client.request(:"#{link['method']}", link['href'],
{}, {})
@current_state = client.send(:"#{item.name}",
item['id']).state
obj.instance_eval do |o|
def state
@current_state
end
end
end
end
define_method :actions do
actions.collect { |a| a.first }
end
define_method :actions_urls do
urls = {}
actions.each { |a| urls[a.first] = a.last }
urls
end
would become (ignoring the 'state' method for now):
when "actions":
attribute.xpath('link').each do |link|
obj.add_action(link['rel'].sanitize, link['method'],
link[:href])
end
There is much more the base class would need to do in order to satisfy
all of what xml_to_class, base_object, and other methods on the client
set up, but you would end up with an implementation that would be more
readable and more easily tested.
Absolutely agree.
-- Michal
--
--------------------------------------------------------
Michal Fojtik, [email protected]
Deltacloud API: http://deltacloud.org
--------------------------------------------------------
