Skip to content

MODULES-444-Add concat multiple#374

Merged
hunner merged 8 commits intopuppetlabs:masterfrom
petems:MODULES-444-add_concat_multiple
Dec 17, 2014
Merged

MODULES-444-Add concat multiple#374
hunner merged 8 commits intopuppetlabs:masterfrom
petems:MODULES-444-add_concat_multiple

Conversation

@petems
Copy link
Copy Markdown
Contributor

@petems petems commented Dec 1, 2014

There was a request to enable concat to take multiple arguments.

https://tickets.puppetlabs.com/browse/MODULES-444

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not extend the existing function, or would that conflict?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went for a soflty approach at first, because I wasn't sure how PL was dealing with backwards compatibility on new features on functions.

If it's cool to change so concat could take > 2 arguments I can definitely refactor this PR to be on the original function and DRY up the code a bit 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored back onto the original function 💃

Peter Souter added 8 commits December 4, 2014 14:12
`concat` can now take multiple arguments
`concat` should be able to concat arrays and primitives
Acceptance test to take multiple arrays for concatenation
Also add extra test for just 1 argument
This is the core change, we now go through the array and add it to the first element, instead of just two arguments.
@igalic
Copy link
Copy Markdown
Contributor

igalic commented Dec 9, 2014

w00t!


will review in a second!

@igalic
Copy link
Copy Markdown
Contributor

igalic commented Dec 9, 2014

look good to me!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we even do this check? We can just cast everything to an array... so it's silly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

hunner added a commit that referenced this pull request Dec 17, 2014
@hunner hunner merged commit 8726caf into puppetlabs:master Dec 17, 2014
@petems petems deleted the MODULES-444-add_concat_multiple branch December 18, 2014 03:37
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops, this should be != 9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@petems yep, just found that, about to PR :)

@underscorgan
Copy link
Copy Markdown
Contributor

see #389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants