Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Code cleanup - no actual change
Before
Code build 2 arrays then uses one to prune the other before merging them. The first array gets precedence
After
Code builds one array, as a property, only setting not-already-set values to achieve the same precedence.
Technical Details
This code has been doing my head in because it is just like 'construct complex array & then another
complex array & iterate them together in nasty ways.
However, after another round of misery I'm pretty sure that this cleanup works. Basically the whole removed
section of code amounts to 'the array built in the first pass takes precedence over the one in the
second pass'. By using a property & only setting the array values when not-yet-set we can
allow the second pass to not clobber the first while populating it rather than as follow up wrangling
@monishdeb I think this gets us closer to readable code which I want before adding more logic complexity in as I find the mergeSameAddress stuff really bad.
Comments
Code is currently unreachable due to https://lab.civicrm.org/dev/core/issues/1421
But
testMergeSameAddress
passes through it with this patchNote the tests need a couple of minor tweaks to still pass after that is merged. We would also get the change of behaviour described in https://lab.civicrm.org/dev/core/issues/1421 so it's cleanup before fixes here