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

problem handling arrays #16

Open
marcelh-gh opened this issue Feb 14, 2017 · 6 comments
Open

problem handling arrays #16

marcelh-gh opened this issue Feb 14, 2017 · 6 comments

Comments

@marcelh-gh
Copy link

case:
combining two files (in the order file1, file2) which both contain a key "a" which contains an array in both files (the array in file2 is a subset of the one in file1)

results:
1.
with the concatArrays option set to true the result is as expected: the "a" keys are combined and the joint array consists of all the entries from both source arrays in the order file1, file2.
2.
with the concatArrays option set to false however the result is not as I would expect.
my expectation is to find just the array entries of file 2 (overwriting the array from file1).
instead the result seems to be a either just the array from file 1 or maybe a merge of the two, which in this scenario happens to be the array from file 1

@marcelh-gh
Copy link
Author

as an example:

"testarray" in key "a" in file 1 is (1,2,3,4)
"testarray" in key "a" in file 2 is (0,1,2)

what "testarray" in the combined file should be is (0,1,2).
instead it is (0,1,2,4)

@joshswan
Copy link
Owner

joshswan commented Feb 14, 2017

This appears to be the default behavior of lodash's mergeWith function as array values are merge key by key.

For now, I've added a mergeArrays option to the 1.0.0 release I just published which can be used to disable array merging entirely (README has been updated as well). I checked and it solves the issue you described above. However, it also means that given two arrays filled with objects, the objects won't be merged, but rather the second array will entirely replace the first. I'll have to give that issue some thought.

@marcelh-gh
Copy link
Author

first of all: thanks a lot for the super quick response!
I have checked 1.0.0 and it works perfectly. much appreciated. 👍

@marcelh-gh
Copy link
Author

two more remarks:

  1. yesterday I had a quick look at the code and already suspected the lodash routine being the problem. what is strange to me though is that the merging does not even merge the values. it rather overwrites the positions in the first array with the values of the second (in case there is a value for that position in the second array). that's why in my above example the result is (0,1,2,4) instead of (0,1,2,3,4). I can't see when this behavior would be actually useful(?). I haven't tried yet but I assume this will also happen if the array contained objects in which case the current behavior would even make less sense to me.

  2. I can understand your concern that with this new setting enabled, objects within arrays also won't be merged anymore. maybe you can implemented an additional sub-option which will deactivate merging only for arrays containing values. that way all use cases could be handled and because the checking would only happen with that sub-option flag active, there would be no possible performance penalty for users who are happy with the old behavior

@marcelh-gh
Copy link
Author

ok...I did quickly test the array merging behavior for two arrays containing objects. It actually works properly!

so my (1) in the last post only is a problem for arrays containing values. I am assuming that's because lodash just takes the numeric array index as the key which results in the observed behavior.

bottom line: for arrays with keyed objects the old behavior is just fine.
therefore I think my proposal (2) of a sub-option to deactivate merging for value arrays only would be a sustainable solution :-)

@joshswan
Copy link
Owner

joshswan commented Feb 15, 2017

Glad it's working for you for now, and thanks for looking into the issues more closely! Your solution is along the lines of what I was thinking would be required, and I think it's probably the best and most flexible option. I'll leave this issue open to remind me and maybe invite some PRs.

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

2 participants