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

mergeOverwrite documentation unclear about sideeffects #997

Open
kmindi opened this issue Dec 14, 2020 · 3 comments
Open

mergeOverwrite documentation unclear about sideeffects #997

kmindi opened this issue Dec 14, 2020 · 3 comments

Comments

@kmindi
Copy link

kmindi commented Dec 14, 2020

The documentation suggests that only the newdict is created, yet the function in fact changes the passed $dest as well. This is not clearly visible from the description of this function and should be added.

E.g. a developer understands a function f(a,b) -> c that a and b are read and only c is something new/created/calculated.

Side effects are dangerous and must be clearly communicated.

https://github.com/helm/helm-www/blob/90ff5b8fae8616dc363bb09990fbc97876bfc92b/content/en/docs/chart_template_guide/function_list.md

@bridgetkromhout
Copy link
Member

The docs in question: https://github.com/helm/helm-www/blob/master/content/en/docs/chart_template_guide/function_list.md#mergeoverwrite-mustmergeoverwrite

I see this line about dest being overwritten; is that where it's unclear? How would you suggest rewording it? Thanks.

Merge two or more dictionaries into one, giving precedence from right to left, effectively overwriting values in the dest dictionary:

@bacongobbler
Copy link
Member

It is also worth noting that this documentation is ripped straight from Sprig's documentation. So any changes that need to be clarified here should also be provided upstream.

https://github.com/Masterminds/sprig/blob/master/docs/dicts.md#mergeoverwrite-mustmergeoverwrite

@kmindi
Copy link
Author

kmindi commented Dec 15, 2020

@bridgetkromhout the line you mentioned is ambiguous: Yes it says that "effectively overwriting values in the dest dictionary", which can mean two things:

  1. that the returned dictionary of the mergeOverwrite function contains all the entries from dest overwritten with the values from all sources
  2. that the dictionary passed to the mergeOverwrite function will be overwritten in-place with the entries from the values from all sources (this one is the sideeffect version that is not expected)

My suggestion would be the following:

Note/Side effect: The dest dictionary will be overwritten in-place by this operation. If you want to keep the original dest you need to pass a copy of it (e.g. with mergeOverwrite (deepCopy $dest) $source1 $source2)

@bacongobbler If we have a merged PR/consensus here we can provide that to upstream

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

No branches or pull requests

3 participants