Skip to content

Conversation

jburnham
Copy link

per puppetlabs/puppetlabs-mysql#274 and for issue 20200 I am adding a recursive merge function to stdlib. Since replacing the normal merge with this new functionality can cause unexpected consequences, this is being added as a new function (rmerge).

@apenney
Copy link

apenney commented Sep 24, 2013

I was asked to call this deep_merge(), any chance youy can rename it? :)

describe 'when calling deep_merge from puppet' do
it "should not compile when no arguments are passed" do
pending("Fails on 2.6.x, see bug #15912") if Puppet.version =~ /^2\.6\./
Puppet[:code] = '$x = deep_merge()'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no testing expert (believe me) but isn't it easier to just call the function directly rather than compile a catalog? It's a lot of overhead just to call a function and test the return like:

Puppet::Parser::Functions.function("deep_merg").with(foo).should == x

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a copy paste job from merge.rb with some additional tests for recursive hashes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahah, fair enough.

@jeffmccune
Copy link
Contributor

Thank you for contributing!

Housekeeping - Commit Message

On the point of housekeeping, please amend the commit message to include a description of the behavior without the patch applied, why this behavior is a problem, and then describe how the change addresses the problem. If the change addresses an error, please consider including the error message verbatim so it's clear if the problem resurfaces in the future. The first line of the commit message should be an imperative statement in the present tense. Change rather than changed or changing for example.

Second, please squash the two commits in this pull request together in an effort to keep the commit history clean.

In addition, I notice that we don't have a contributor license agreement on file. Please sign the CLA as per instructions in CONTRIBUTING.md. Please note, you don't need to print and mail the agreement. Electronically signing the agreement is sufficient.

The direct link to sign the CLA is: https://projects.puppetlabs.com/contributor_licenses/sign

Once these three things are addressed we'll take another look at merging this new functionality into stdlib.

Thanks again for the contribution,
-Jeff

@jburnham
Copy link
Author

jburnham commented Oct 4, 2013

closing this pull request and adding #189

@jburnham jburnham closed this Oct 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants