Add hash_merge and hash_merge_recursive filters with documentation #12078

Merged
merged 1 commit into from Aug 27, 2015

Conversation

Projects
None yet
4 participants
@amenonsen
Contributor

amenonsen commented Aug 25, 2015

This PR implements the most-requested subset of the earlier (closed) PR
largely unchanged from PR #7872 by @darkk (but the filters are named
differently to address the fact that hash_behaviour=replace doesn't
actually do a shallow merge, as the earlier PR implied).

Closes #7872 by @darkk (hash_merge/hash_replace filters)
Closes #11153 by @telbizov (merged_dicts lookup plugin)

cc: @evanccnyc

@evanccnyc

This comment has been minimized.

Show comment
Hide comment
@evanccnyc

evanccnyc Aug 25, 2015

Contributor

Be nice to have this but I am not sure where @bcoca and @abadger ended up.

Contributor

evanccnyc commented Aug 25, 2015

Be nice to have this but I am not sure where @bcoca and @abadger ended up.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 26, 2015

Member

This looks okay to me. I take it you couldn't get arbitrary numbers of dict params to play well with recursive being an optional keywpord arg?

Member

abadger commented Aug 26, 2015

This looks okay to me. I take it you couldn't get arbitrary numbers of dict params to play well with recursive being an optional keywpord arg?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Aug 26, 2015

Member

i would have preferred something more neutral as some people know what a hash is, some people know them as dictionaries or associative arrays and many people don't know any of the above.

But adding this to the set theory filters has been declined ...

Member

bcoca commented Aug 26, 2015

i would have preferred something more neutral as some people know what a hash is, some people know them as dictionaries or associative arrays and many people don't know any of the above.

But adding this to the set theory filters has been declined ...

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Aug 26, 2015

Contributor

@abadger that's right, I could have done (recursive=False, *terms), but then it wouldn't have been optional; and (*terms, recursive=False) isn't allowed. I could have passed in a list argument and an optional boolean, but at that point the awkwardness seemed to outweigh the benefits.

@bcoca I stuck with hash_merge* in particular because of the correspondence with hash_behaviour in ansible.cfg. Given how many times this feature has been asked for and reimplemented in various different ways, I don't think the nomenclature is a major sticking point.

Contributor

amenonsen commented Aug 26, 2015

@abadger that's right, I could have done (recursive=False, *terms), but then it wouldn't have been optional; and (*terms, recursive=False) isn't allowed. I could have passed in a list argument and an optional boolean, but at that point the awkwardness seemed to outweigh the benefits.

@bcoca I stuck with hash_merge* in particular because of the correspondence with hash_behaviour in ansible.cfg. Given how many times this feature has been asked for and reimplemented in various different ways, I don't think the nomenclature is a major sticking point.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 27, 2015

Member

Looks good. We talked about this at the internal team meeting today and implementation seems good. Just want to rename for @bcoca's preference for something neutral. Either merge() and merge_recursive() or combine() and combine_recursive() were suggested. bcoca also thought this would work for other iterables, not just dictionaries which is why he wanted the more neutral naming. After some discussion he convinced us that hash, dict, and associative array are all jargon (not all of our users know about them) so it would be better to add support for other iterables to these filters and change the name rather than keep the name and keep the filters limited to only dicts.

I can make those changes after merge if you want or you can do it yourself and then I'll merge. Let me know your preference. I think I may have an idea of how to get recursive to work as a parameter too that I might try. Something like:

def combine(*terms, **kwargs):
    recursive = kwargs.get('recursive', False)
    if len(kwargs) > 1 or ('recursive' not in kwargs and len(kwargs) == 1):
        # Error about invalid parameter
Member

abadger commented Aug 27, 2015

Looks good. We talked about this at the internal team meeting today and implementation seems good. Just want to rename for @bcoca's preference for something neutral. Either merge() and merge_recursive() or combine() and combine_recursive() were suggested. bcoca also thought this would work for other iterables, not just dictionaries which is why he wanted the more neutral naming. After some discussion he convinced us that hash, dict, and associative array are all jargon (not all of our users know about them) so it would be better to add support for other iterables to these filters and change the name rather than keep the name and keep the filters limited to only dicts.

I can make those changes after merge if you want or you can do it yourself and then I'll merge. Let me know your preference. I think I may have an idea of how to get recursive to work as a parameter too that I might try. Something like:

def combine(*terms, **kwargs):
    recursive = kwargs.get('recursive', False)
    if len(kwargs) > 1 or ('recursive' not in kwargs and len(kwargs) == 1):
        # Error about invalid parameter
Add a combine filter with documentation
This is based on some code from (closed) PR #7872, but reworked based on
suggestions by @abadger and the other core team members.

Closes #7872 by @darkk (hash_merge/hash_replace filters)
Closes #11153 by @telbizov (merged_dicts lookup plugin)
@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Aug 27, 2015

Contributor

I've pushed a revised commit along the lines explained by @abadger.

Contributor

amenonsen commented Aug 27, 2015

I've pushed a revised commit along the lines explained by @abadger.

abadger added a commit that referenced this pull request Aug 27, 2015

Merge pull request #12078 from amenonsen/hash_merge
Add hash_merge and hash_merge_recursive filters with documentation

@abadger abadger merged commit db4a96a into ansible:devel Aug 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 27, 2015

Member

Phew! Thank you to everyone who worked on this problem! And thank you to @amenonsen for seeing it through to completion!

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

Member

abadger commented Aug 27, 2015

Phew! Thank you to everyone who worked on this problem! And thank you to @amenonsen for seeing it through to completion!

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

@amenonsen amenonsen deleted the amenonsen:hash_merge branch Aug 31, 2015

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