From 33b5580ef6b6c851beb6852e56659afea8bb0b04 Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 19 Apr 2011 18:22:03 -0700 Subject: maint: move method comments outside the comment. The comment discussing the purpose of the wrapper and related details rightly belongs outside the method; move it there so it doesn't perturb the functional changes that follow. Reviewed-By: Max Martin --- lib/puppet/interface/action.rb | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) (limited to 'lib/puppet/interface') diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index 23366b407..e644d8999 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -129,27 +129,26 @@ class Puppet::Interface::Action # @face.send(name, *args, &block) # end + + # We need to build an instance method as a wrapper, using normal code, to be + # able to expose argument defaulting between the caller and definer in the + # Ruby API. An extra method is, sadly, required for Ruby 1.8 to work since + # it doesn't expose bind on a block. + # + # Hopefully we can improve this when we finally shuffle off the last of Ruby + # 1.8 support, but that looks to be a few "enterprise" release eras away, so + # we are pretty stuck with this for now. + # + # Patches to make this work more nicely with Ruby 1.9 using runtime version + # checking and all are welcome, provided that they don't change anything + # outside this little ol' bit of code and all. + # + # Incidentally, we though about vendoring evil-ruby and actually adjusting + # the internal C structure implementation details under the hood to make + # 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 def when_invoked=(block) - # We need to build an instance method as a wrapper, using normal code, to - # be able to expose argument defaulting between the caller and definer in - # the Ruby API. An extra method is, sadly, required for Ruby 1.8 to work. - # - # In future this also gives us a place to hook in additional behaviour - # such as calling out to the action instance to validate and coerce - # parameters, which avoids any exciting context switching and all. - # - # Hopefully we can improve this when we finally shuffle off the last of - # Ruby 1.8 support, but that looks to be a few "enterprise" release eras - # away, so we are pretty stuck with this for now. - # - # Patches to make this work more nicely with Ruby 1.9 using runtime - # version checking and all are welcome, but they can't actually help if - # the results are not totally hidden away in here. - # - # Incidentally, we though about vendoring evil-ruby and actually adjusting - # the internal C structure implementation details under the hood to make - # 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 internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym file = __FILE__ + "+eval" -- cgit From 5d7ef5caf30a0c5b3253340c5f2722e51c56c75e Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 19 Apr 2011 20:23:27 -0700 Subject: (#7062) better argument handling in the action wrapper methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We previously used *args to collect all arguments to the action when_invoked block, then tried vaguely to massage some little bits of them into the right shape. Methods defined with blocks, in Ruby 1.8, also have some fun behaviours. The most special is that if you pass more than one argument to a block defined with only one Ruby will automatically coerce the arguments into an array – and this is preserved when it is bound to a method. This led to routine situations where we would pass the wrong number of arguments to the block because, say, the user gave an extra argument on the command line. Instead of failing this would transmogrify the arguments in counterintuitive ways, and end up with horrible stack traces when that interacted badly with the code as written. Now, instead, we work out the right argument format based on the arguments that the when_invoked block takes. This gives much better (albeit perhaps not so user friendly) behaviour at the interface level. Which is, at least, consistent with other Ruby API. Reviewed-By: Max Martin --- lib/puppet/interface/action.rb | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) (limited to 'lib/puppet/interface') diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index e644d8999..08bc0a345 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -151,15 +151,37 @@ class Puppet::Interface::Action def when_invoked=(block) internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym - file = __FILE__ + "+eval" - line = __LINE__ + 1 + + arity = block.arity + if arity == 0 then + # This will never fire on 1.8.7, which treats no arguments as "*args", + # but will on 1.9.2, which treats it as "no arguments". Which bites, + # because this just begs for us to wind up in the horrible situation + # where a 1.8 vs 1.9 error bites our end users. --daniel 2011-04-19 + raise ArgumentError, "action when_invoked requires at least one argument (options)" + elsif arity > 0 then + range = Range.new(1, arity - 1) + decl = range.map { |x| "arg#{x}" } << "options = {}" + optn = "" + args = "[" + (range.map { |x| "arg#{x}" } << "options").join(", ") + "]" + else + range = Range.new(1, arity.abs - 1) + decl = range.map { |x| "arg#{x}" } << "*rest" + optn = "rest << {} unless rest.last.is_a?(Hash)" + if arity == -1 then + args = "rest" + else + args = "[" + range.map { |x| "arg#{x}" }.join(", ") + "] + rest" + end + end + + file = __FILE__ + "+eval[wrapper]" + line = __LINE__ + 2 # <== points to the same line as 'def' in the wrapper. wrapper = < Date: Thu, 21 Apr 2011 10:38:25 -0700 Subject: maint: more robust listing of valid faces. We used to treat anything with a top level key in the faces hash as a valid face; it makes more sense to filter that only to things that have at least one implementation. Previously we had to be super-careful not to accidentally touch the top level for an invalid face, which set us up for future failure when someone wasn't careful enough; now we can cope with that. Paired-With: Max Martin --- lib/puppet/interface/face_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/puppet/interface') diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb index 591471d4b..49e007ec0 100644 --- a/lib/puppet/interface/face_collection.rb +++ b/lib/puppet/interface/face_collection.rb @@ -24,7 +24,7 @@ module Puppet::Interface::FaceCollection end end end - return @faces.keys + return @faces.keys.select {|name| @faces[name].length > 0 } end def self.validate_version(version) -- cgit From f17f6bba87519db888854acf7017ddff61f635be Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Wed, 20 Apr 2011 15:51:39 -0700 Subject: (#7183) Implement "invisible glob" version matching for faces "Invisible glob", or "prefix", version matching means that when you specify a version string to use you can specify as little as one version number out of the semantic versioning spec. Matching is done on the prefix; an omitted number is treated as "anything" in that slot, and we return the highest matching versioned face by that spec. For example, given the set of versions: 1.0.0, 1.0.1, 1.1.0, 1.1.1, 2.0.0 The following would be matched: input matched 1 1.1.1 1.0 1.0.1 1.0.1 1.0.1 1.0.2 fail - no match 1.1 1.1.1 1.1.1 1.1.1 1.2 fail - no match --- lib/puppet/interface/face_collection.rb | 58 ++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 23 deletions(-) (limited to 'lib/puppet/interface') diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb index 49e007ec0..6e6afc545 100644 --- a/lib/puppet/interface/face_collection.rb +++ b/lib/puppet/interface/face_collection.rb @@ -31,12 +31,14 @@ module Puppet::Interface::FaceCollection !!(SEMVER_VERSION =~ version.to_s) end + def self.semver_to_array(v) + parts = SEMVER_VERSION.match(v).to_a[1..4] + parts[0..2] = parts[0..2].map { |e| e.to_i } + parts + end + def self.cmp_semver(a, b) - a, b = [a, b].map do |x| - parts = SEMVER_VERSION.match(x).to_a[1..4] - parts[0..2] = parts[0..2].map { |e| e.to_i } - parts - end + a, b = [a, b].map do |x| semver_to_array(x) end cmp = a[0..2] <=> b[0..2] if cmp == 0 @@ -47,18 +49,38 @@ module Puppet::Interface::FaceCollection cmp end - def self.[](name, version) - @faces[underscorize(name)][version] if face?(name, version) + def self.prefix_match?(desired, target) + # Can't meaningfully do a prefix match with current on either side. + return false if desired == :current + return false if target == :current + + # REVISIT: Should probably fail if the matcher is not valid. + prefix = desired.split('.').map {|x| x =~ /^\d+$/ and x.to_i } + have = semver_to_array(target) + + while want = prefix.shift do + return false unless want == have.shift + end + return true end - def self.face?(name, version) + def self.[](name, version) name = underscorize(name) + get_face(name, version) or load_face(name, version) + end - # Note: be careful not to accidentally create the top level key, either, - # because it will result in confusion when people try to enumerate the - # list of valid faces later. --daniel 2011-04-11 - return true if @faces.has_key?(name) and @faces[name].has_key?(version) + # get face from memory, without loading. + def self.get_face(name, desired_version) + return nil unless @faces.has_key? name + return @faces[name][:current] if desired_version == :current + + found = @faces[name].keys.select {|v| prefix_match?(desired_version, v) }.sort.last + return @faces[name][found] + end + + # try to load the face, and return it. + def self.load_face(name, version) # We always load the current version file; the common case is that we have # the expected version and any compatibility versions in the same file, # the default. Which means that this is almost always the case. @@ -104,17 +126,7 @@ module Puppet::Interface::FaceCollection # ...guess we didn't find the file; return a much better problem. end - # Now, either we have the version in our set of faces, or we didn't find - # the version they were looking for. In the future we will support - # loading versioned stuff from some look-aside part of the Ruby load path, - # but we don't need that right now. - # - # So, this comment is a place-holder for that. --daniel 2011-04-06 - # - # Note: be careful not to accidentally create the top level key, either, - # because it will result in confusion when people try to enumerate the - # list of valid faces later. --daniel 2011-04-11 - return !! (@faces.has_key?(name) and @faces[name].has_key?(version)) + return get_face(name, version) end def self.register(face) -- cgit