don't fail on undef variable in merge

[mhellmic]

I'll explain the change, why I'd like it, and how I implemented it in the following.
EDIT: sorry, it should only concern commit 3077d26, not all three.

Proposed change:
if merge gets passed an undefined variable, it doesn't fail with an exception, but instead silently ignores the value

Use case:
In my configuration file, one parameter can be repeatedly set with different values. Both key and value are variable, so I use a hash. Based on the input, different values are used, so I create multiple hashes, combine them with merge and then pass them to the template as one (the template is part of a more generic puppet module and should not know about the different internal hashes I created).

Because some value are used, merge fails and I would have to check first for all combinations and then call merge in the right way.
I have three hashes to combine so I already have to check for nine combinations, for somebody with more it's even less feasible.

I'll sketch the code here:

if one {
    var1 = { abc => 123, bcd => 234 }
}
if two {
    var2 = { def => 456 }
}
if three { ... }

var = merge(var1, var2, var3) # might fail if one is undef

Code change:
Add a check for .empty? for every argument. Good, because it filters everything that is not empty: string, lists, ... . Bad, because you could get away with passing a wrong type in some cases, get an exception in others. Unfortunately, puppets undef doesn't seem to map to anything useful in ruby. We can use arg == "", but that's only slightly better.
Any other ideas are most welcome!

[adrienthebo]

Thanks for this contribution!

With respect to your observation about calling .empty? on the arguments, if something like an Integer is passed that doesn't respond to .empty? then this will fail with a semi-cryptic argument. I think it would be better to be more restrictive, and do something like this:

next if arg.is_a? String and arg.empty?

In addition this adds new behavior, so this should have tests to help prevent regressions. The tests at https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/spec/unit/puppet/parser/functions/merge_spec.rb#L27 could be duplicated and modified to ensure that this code behaves as expected. If you would like help with this please let me know; I'm finch in #puppet on irc.freenode.net and I'm generally available from 9:00 AM - 5:00 PM GMT -7. Thanks again!

[adrienthebo]

I noticed that this hasn't seen any progress in about a week. Is there a chance that you'll be able to work on this soon? No rush, just checking in.

[mhellmic]

Hi Adrien, I'll be able to do that next week :)

[mhellmic]

Hi Adrien,

I incorporated your suggestion and created a new test. I also extended the 'should accept only hashes' test to see if an integer fails with the right exception. Is that ok there or should it be another test?

[adrienthebo]

Please pardon the delay; I've been working on another bug that took longer to handle than anticipated. This looks good!

summary: merged into master in dad3a29; this should be released in the next release of the 4.x branch. Thanks again for the contribution!