-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: Add possibility to recursive merge data in Context #307
feat: Add possibility to recursive merge data in Context #307
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Like the feature!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (apart the question about array_merge_recursive)
I can't figure what the best default value for $recursive, both are legit in my mind.
Should we throw a warning in case $recursive is given but keepExisting is false ?
IDK, I leave it up to you to decide on the potential impact of existing contexts. Yes, the current implementation doesn't care about the value when |
Rebased and added a conditional check when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 thanks
Done, fixed the changelog conflict, if no more feedback it's ready 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🎉
Description
Overview
This pull request addresses the need for improved array merging behavior, specifically focusing on preserving nested values while merging arrays.
Examples
defaultContext
FunctionFor example, the
defaultContext
function is updated to return aContext
object with a nested array. Thenested
key contains an array with amerge
key, which in turn contains another array.This structure is designed to demonstrate the merging behavior of nested arrays.
The
replaced
key is intended to be replaced, while thevalue
key is expected to be preserved.productionContext
FunctionThe
productionContext
function is based on thedefaultContext
function, with the intention of modifying theContext
object to reflect a production environment.In the case of the
nested
array, thereplaced
key is replaced with a new value, and a new key is added to thekey
array.Outcome
Actual
In the case of actual behavior, the
Context
object returned by theproductionContext
function is as follows (or now withrecursive
set tofalse
):New Behavior
With the updated behavior, the
Context
object returned by theproductionContext
function is as follows, andrecursive
set totrue
:Recursive Merging
Note
Actually the
recursive
option is by defaulttrue
.Is this a good idea to change the default behavior? or better to set
false
by default and let the user decide to set it totrue
?