(MODULES-2456) Modify union to accept more than two arrays #507
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jetroid
changed the title
| raise(Puppet::ParseError, "union(): Wrong number of arguments " + | ||
| "given (#{arguments.size} for 2)") if arguments.size != 2 | ||
| # Check that 2 or more arguments have been given ... | ||
| raise(Puppet::ParseError, "concat(): Wrong number of arguments " + |
There was a problem hiding this comment.
you mean 'union', not 'concat' here?
|
Hi, this seems to be a worthwhile addition. There seems to be one typo in the error message and the actual function body could be simplified significantly. I've pointed that out in code comments. Cheers! |
|
Thanks for the criticism, DavidS. Your comments are all valid. Have made the necessary changes and pushed. |
|
Sweet. Would you mind squashing it into a single commit, just so the history is nice and clean? http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html |
Add spec tests to test the new functionality: *Case for 3 arrays. *Case for 4 arrays. Modify README to note new functionality. This is for issue MODULE-2456, follow the precedent of MODULE-444. This change allows union to be much more useful, unioning many arrays in one line rather than in n lines. Additionally, as this is only added functionality, and does not affect the 2 array case that all modules currently using array are using, it should not affect any existing modules utilizing union. This is now useful, for example, for merging many arrays of resources (eg: packages.) to generate just one list with no duplicates, to avoid duplicate resource declarations.
|
No worries. Complete. Thanks! |
|
Thank you for your contribution! |
DavidS
added a commit
that referenced
this pull request
Aug 24, 2015
(MODULES-2456) Modify union to accept more than two arrays
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add spec tests to test the new functionality:
*Case for 3 arrays.
*Case for 4 arrays.
Modify README to note new functionality.
This is for issue MODULE-2456, follow the precedent of MODULE-444.
This change allows union to be much more useful, unioning many arrays
in one line rather than in n lines. Additionally, as this is only added
functionality, and does not affect the 2 array case that all modules
currently using array are using, it should not affect any existing
modules utilizing union.
This is now useful, for example, for merging many arrays of resources
(eg: packages.) to generate just one list with no duplicates, to avoid
duplicate resource declarations.