Skip to content

make the merge function more precise#24032

Merged
jbardin merged 2 commits intomasterfrom
jbardin/map-funcs
Feb 12, 2020
Merged

make the merge function more precise#24032
jbardin merged 2 commits intomasterfrom
jbardin/map-funcs

Conversation

@jbardin
Copy link
Copy Markdown
Member

@jbardin jbardin commented Feb 4, 2020

This PR implements 2 changes to the merge function.

  • Rather than always defining the merge return type as dynamic, return
    a precise type when all argument types match, or all possible object
    attributes are known.
  • Always return a value containing all keys when the keys are known.
    This allows the use of merge output in for_each, even when keys are yet
    to be determined.

Fixes #24024

This also handles the bulk of the concerns from #23052 and comments in #22735. While those had other confounding factors that led us astray, the common thread is that users expect merge to return what known keys it has, which is what a for expression would be able to do.

@jbardin jbardin added the config label Feb 4, 2020
@jbardin jbardin requested a review from a team February 4, 2020 23:35
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Feb 4, 2020
}),
false,
},
{ // attr a type and value is overridden
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.

^^ I think we should add this (type and value being overridden) as an example to the documentation, what do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, it looks like this was never documented to work on anything by maps! Hard to roll that back now since users very quickly discovered that arbitrary combinations of objects and maps could be merged.

I guess we need to update the whole doc page on this

Copy link
Copy Markdown
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Nice! I think we should add a couple of examples to the documentation - there's a bit in here that might not be intuitive, like overriding an attribute's type - but that's not something to block this PR

This PR implements 2 changes to the merge function.
 - Rather than always defining the merge return type as dynamic, return
 a precise type when all argument types match, or all possible object
 attributes are known.
 - Always return a value containing all keys when the keys are known.
 This allows the use of merge output in for_each, even when keys are yet
 to be determined.
@jbardin jbardin merged commit a765d69 into master Feb 12, 2020
@jbardin jbardin deleted the jbardin/map-funcs branch February 12, 2020 15:51
`merge` takes an arbitrary number of maps and returns a single map that
contains a merged set of elements from all of the maps.
`merge` takes an arbitrary number of maps or objects, and returns a single map
pr object that contains a merged set of elements from all arguments.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Howdy, I was just passing by. Sorry if this is a duplicate:

Watch out pr object I think you meant or object

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, totally missed that

@ghost
Copy link
Copy Markdown

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

config sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Map functions should be able to return known keys

3 participants