2
\$\begingroup\$

I've got a PR to add a feature to a Ruby application (Puppet) to allow Regex detection.

Right now, the method only does comparison on exact string matching:

defaultfor :operatingsystem => :fedora, :operatingsystemmajrelease => ['22', '23', '24']

This is annoying, as we have to add in new values everytime a new Fedora release comes out, for example.

I've extended it to match on a Regex:

defaultfor :operatingsystem => :fedora, :operatingsystemmajrelease => /^2[2-9]$/

The old method looks like this:

def self.fact_match(fact, values)
  values = [values] unless values.is_a? Array
  values.map! { |v| v.to_s.downcase.intern }

  if fval = Facter.value(fact).to_s and fval != ""
    fval = fval.to_s.downcase.intern

    values.include?(fval)
  else
    false
  end
end

This is my current proof-of-concept code, which will match both regex and strings, but it feels off.

def self.fact_match(fact, values)

  values = [values] unless values.is_a? Array

  values.map! do |v|
    if v.is_a? Regexp
      v
    else
      v.to_s.downcase.intern
    end
  end

  if fval = Facter.value(fact).to_s and fval != ""
    fval = fval.to_s.downcase.intern
    if values.any? {|v| v.is_a? Regexp }
      regex_match = Regexp.union(values)
      fval =~ regex_match
    else
      values.include?(fval)
    end
  else
    false
  end
end

I feel like there's a much easier way of detecting if it's a Regex or not.

\$\endgroup\$
2
\$\begingroup\$

You're working too hard, trying to distinguish strings from regular expressions. Rather, you should take advantage of the fact that the === operator for a RegExp and the === operator for a String both express the idea of a "match", which is what you want here.

I don't understand why you need to #intern all the strings.

I don't see why the fval = Facter.value(fact).to_s assignment appears inside an if condition. The result of the assignment would only be false if Facter.value(fact).to_s is false or nil, which can never happen. (Even nil.to_s results in an empty string.)

To treat values as an array regardless of whether a scalar or an array was passed in, you can use the [*values] idiom.

def self.fact_match(fact, values)
  fact_val = Facter.value(fact).to_s.downcase
  fact_val != "" and [*values].any? { |v| v === fact_val }
end

In addition to supporting literal case-insensitive matches and regular expression matches, also consider supporting Ranges.

\$\endgroup\$
  • \$\begingroup\$ That's a lot cleaner, thanks! Makes a lot more sense. Looking at the commit for that code, it looks like that extra intern logic was added when each defaultfor became a hash of multiple different values. Indeed, when I change the logic to the code given, everything works ok except for the unit tests for multiple defaultfor values... I'll have to investigate deeper! \$\endgroup\$ – Peter Souter Dec 14 '16 at 14:03
  • \$\begingroup\$ I'm gladyou answered with the === idiom. It's one of my favorites. Also, the [*values] idiom can also be expressed as Array(values) \$\endgroup\$ – Wayne Conrad Dec 19 '16 at 12:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.