laradji / zabbix

Zabbix chef cookbook
Apache License 2.0
91 stars 124 forks source link

Remove define_singleton_method in favor of opening multiple eigen classes for 1.8.7 support #77

Closed andrewGarson closed 11 years ago

andrewGarson commented 11 years ago

This is used to create "enum" classes so that we can enforce a bit of sanity around all the magic numbers that zabbix uses in the api. I would rather not have the lwrps accepting Fixnums for things like "type" because then a person trying to use them basically has to have the api docs open instead of just having a working knowledge of zabbix.

@laradji I can "fix" this to work on 1.8, but it has a bit of strangeness to it. Basically, in order to add the methods without define_singleton_method i have to use the following trick:

eigen_class = class << self; self; end
eigen_class.send(:define_method .....)

instead of

self.send(:define_singleton_method)

The difference is that each call to the enum method (1 per value in the enumeration) will open a new eigen class and put one method into it

class Chicken
  include Enumeration
  enum :tuna, 0
  enum :laser_beams, 1
end

self#define_singleton_method will produce [ Chicken ] < [ Chicken' ] < [ Object ] with #tuna and #laser_beams eigen_class#define_method will produce [ Chicken ] < [ Chicken' ] < [ Chicken'' ] < [ Object ] with one method in each ' class

This should not cause any functional difference (except a slightly slower method resolution) but could cause strangeness with stacktraces or debugging.

andrewGarson commented 11 years ago

Should fix #76

guilhem commented 11 years ago

Good for me, I was also working on something like this:

self.class.instance_eval do
  define_method(name) { @enumeration_values[name] }
end

But your solution seems better.

Can you add a comment in code (for futur contributor to know that this is for 1.8 compatibility) and you miss an update in metadata.rb ;)

andrewGarson commented 11 years ago

@guilhem i'm not 100% certain that code works. I believe it would pollute the Class class (not 100% certain without testing).

Just so everybody is aware, the reason I went with this complex enum stuff instead of just using Constants (as would be normal) is that I am generating recipes off of xml template dumps and needed to maintain the name<->fixnum mapping in memory.

I'll add that comment.

guilhem commented 11 years ago

@andrewGarson no more error on 1.8 for me :)

guilhem commented 11 years ago

can you remove the version change?

andrewGarson commented 11 years ago

oops

the orchestrator we're building required me to bump the version to test quickly. Fixed

guilhem commented 11 years ago

It's good enough for me to merge. @laradji do you want to do this? :)

laradji commented 11 years ago

let's go.