Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update documentation of the combine filter #82329

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

sschmittsva
Copy link

SUMMARY

I've updated the documentation of the combine filter. Changes:

  • add to the short description that two or more dictionaries can be combined
  • clarify in the description the notion of old/new (used later in the documentation for list_merge)
  • add that the input can be a list of dictionaries
  • extend the examples
ISSUE TYPE
  • Docs Pull Request

@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label Dec 1, 2023
@webknjaz
Copy link
Member

webknjaz commented Dec 1, 2023

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Dec 7, 2023
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Can we keep the existing examples as one liners, you can use your more complex ones as static dict definitions just above the combine template itself.

@sschmittsva
Copy link
Author

Can we keep the existing examples as one liners

Sure. I'll readd them.

you can use your more complex ones as static dict definitions just above the combine template itself.

Sorry, I don't understand what you mean?

@jborean93
Copy link
Contributor

Sorry, I don't understand what you mean?

I was just trying to say that it's best to keep the actual dictionary definitions just above the combine example rather than put them all at the top before using the combine template.

@sschmittsva
Copy link
Author

Sorry, I don't understand what you mean?

I was just trying to say that it's best to keep the actual dictionary definitions just above the combine example rather than put them all at the top before using the combine template.

Thanks. Please have a look again.

# defaults => {'a':{'b':3, 'c':4}, 'd': 5}
# customization => {'a':{'c':20}}
# final => {'a':{'b':3, 'c':20}, 'd': 5}
final: "{{ defaults | ansible.builtin.combine(customization, recursive=true) }}"

ab:
Copy link
Member

Choose a reason for hiding this comment

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

keep the defs as on liners to make them readable with the examples

positional: _input, _dicts
options:
_input:
description: First dictionary to combine.
description: First dictionary to combine or a list of dictionaries to combine.
Copy link
Contributor

Choose a reason for hiding this comment

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

@sschmittsva AIUI _input is always a single parameter, in which the function receives everything that's passed into the filter (i.e. before | combine(...)), so it only takes a single dict

Copy link
Author

Choose a reason for hiding this comment

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

That's what the former documentation wants you to think :) But it allows to receive a list of dicts, cf.

# allow the user to do `[dict1, dict2, ...] | combine`
and the example list_combine_ab_bc_cd.

Copy link
Member

Choose a reason for hiding this comment

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

it was never intended for the input to be a list of dictionaries, for a case like this i would use map/select/reject filters on top. I'm of a mind to 'fix' combine to follow the normal filter semantics.

Copy link
Contributor

Choose a reason for hiding this comment

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

That 'fix' would push more work onto users. For example, you can currently do

# Example 1
[defaults_dict] | product(list_of_dicts) | map("combine")

With your suggested 'fix', users would need to do

# Example 2
list_of_dicts | map("combine", defaults_dict, reverse=true)

except that won't work because combine lacks the reverse parameter to cause the dicts to be combined right-to-left instead of the heretofore left-to-right order. (In fact, I stumbled across #82329 while researching issues in prep for proposing just such an RFE.)

With such a fix in place, I don't see how one would accomplish what "Example 1" does. Would it not be better to 'fix' other filters that can't currently work this way?

Copy link
Member

Choose a reason for hiding this comment

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

its 1 to 100 and also many filters are implemented in other projects, I'm not going to jinja2 and every collection to modify them, its always simpler to fix the outlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also fair. I may have misunderstood how the 'fix' you're talking about would percolate up to users. I do want to "get it" though, particularly because I maintain some internal filters for our group, and I'd like to do things the Right Way™.

Thanks for your perspective. This discussion would be better held in the forum. See you there.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 2, 2024
@sschmittsva
Copy link
Author

Is there anything else you would like to have changed?

@samccann
Copy link
Contributor

@bcoca can you take another look (esp #82329 (comment) to see if it's accurate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants