-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add option to clone objects before merge. Fixes #28 issue. #44
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
de0b160
Add option to clone source and target. Fixes #28 issue.
gagan-bansal db6a5c7
Clone option rechecked and corrected
gagan-bansal c5b5b07
Merge remote-tracking branch 'upstream/master'
gagan-bansal 806415e
clone option test cases improved
gagan-bansal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,6 +106,67 @@ test('should add nested object in target', function(t) { | |
t.end() | ||
}) | ||
|
||
test('should clone source and target', function(t) { | ||
var src = { | ||
"b": { | ||
"c": "foo" | ||
} | ||
} | ||
|
||
var target = { | ||
"a": { | ||
"d": "bar" | ||
} | ||
} | ||
|
||
var expected = { | ||
"a": { | ||
"d": "bar" | ||
}, | ||
"b": { | ||
"c": "foo" | ||
} | ||
} | ||
|
||
var merged = merge(target, src, {clone: true}) | ||
|
||
t.deepEqual(merged, expected) | ||
|
||
t.notEqual(merged.a, target.a) | ||
t.notEqual(merged.b, src.b) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two tests should probably assert that |
||
t.end() | ||
}) | ||
|
||
test('should not clone source and target', function(t) { | ||
var src = { | ||
"b": { | ||
"c": "foo" | ||
} | ||
} | ||
|
||
var target = { | ||
"a": { | ||
"d": "bar" | ||
} | ||
} | ||
|
||
var expected = { | ||
"a": { | ||
"d": "bar" | ||
}, | ||
"b": { | ||
"c": "foo" | ||
} | ||
} | ||
|
||
var merged = merge(target, src) | ||
t.equal(merged.a, target.a) | ||
t.equal(merged.b, src.b) | ||
|
||
t.end() | ||
}) | ||
|
||
test('should replace object with simple key in target', function (t) { | ||
var src = { key1: 'value1' } | ||
var target = { | ||
|
@@ -176,6 +237,30 @@ test('should work on array properties', function (t) { | |
t.end() | ||
}) | ||
|
||
test('should work on array properties with clone option', function (t) { | ||
var src = { | ||
key1: ['one', 'three'], | ||
key2: ['four'] | ||
} | ||
var target = { | ||
key1: ['one', 'two'] | ||
} | ||
|
||
var expected = { | ||
key1: ['one', 'two', 'three'], | ||
key2: ['four'] | ||
} | ||
|
||
t.deepEqual(target, { | ||
key1: ['one', 'two'] | ||
}) | ||
var merged = merge(target, src, {clone: true}) | ||
t.notEqual(merged.key1, src.key1) | ||
t.notEqual(merged.key1, target.key1) | ||
t.notEqual(merged.key2, src.key2) | ||
t.end() | ||
}) | ||
|
||
test('should work on array of objects', function (t) { | ||
var src = [ | ||
{ key1: ['one', 'three'], key2: ['one'] }, | ||
|
@@ -202,6 +287,37 @@ test('should work on array of objects', function (t) { | |
t.end() | ||
}) | ||
|
||
test('should work on array of objects with clone option', function (t) { | ||
var src = [ | ||
{ key1: ['one', 'three'], key2: ['one'] }, | ||
{ key3: ['five'] } | ||
] | ||
var target = [ | ||
{ key1: ['one', 'two'] }, | ||
{ key3: ['four'] } | ||
] | ||
|
||
var expected = [ | ||
{ key1: ['one', 'two', 'three'], key2: ['one'] }, | ||
{ key3: ['four', 'five'] } | ||
] | ||
|
||
t.deepEqual(target, [ | ||
{ key1: ['one', 'two'] }, | ||
{ key3: ['four'] } | ||
]) | ||
var merged = merge(target, src, {clone: true}) | ||
t.deepEqual(merged, expected) | ||
t.ok(Array.isArray(merge(target, src)), 'result should be an array') | ||
t.ok(Array.isArray(merge(target, src)[0].key1), 'subkey should be an array too') | ||
t.notEqual(merged[0].key1, src[0].key1) | ||
t.notEqual(merged[0].key1, target[0].key1) | ||
t.notEqual(merged[0].key2, src[0].key2) | ||
t.notEqual(merged[1].key3, src[1].key3) | ||
t.notEqual(merged[1].key3, target[1].key3) | ||
t.end() | ||
}) | ||
|
||
test('should work on arrays of nested objects', function(t) { | ||
var target = [ | ||
{ key1: { subkey: 'one' }} | ||
|
@@ -231,6 +347,20 @@ test('should treat regular expressions like primitive values', function (t) { | |
t.end() | ||
}) | ||
|
||
test('should treat regular expressions like primitive values and should not' | ||
+ ' clone even with clone option', | ||
function (t) { | ||
var target = { key1: /abc/ } | ||
var src = { key1: /efg/ } | ||
var expected = { key1: /efg/ } | ||
|
||
var output = merge(target, src, {clone: true}) | ||
|
||
t.equal(output.key1, src.key1) | ||
t.end() | ||
} | ||
) | ||
|
||
test('should treat dates like primitives', function(t) { | ||
var monday = new Date('2016-09-27T01:08:12.761Z') | ||
var tuesday = new Date('2016-09-28T01:18:12.761Z') | ||
|
@@ -252,6 +382,27 @@ test('should treat dates like primitives', function(t) { | |
t.end() | ||
}) | ||
|
||
test('should treat dates like primitives and should not clone even with clone' | ||
+ ' option', function(t) { | ||
var monday = new Date('2016-09-27T01:08:12.761Z') | ||
var tuesday = new Date('2016-09-28T01:18:12.761Z') | ||
|
||
var target = { | ||
key: monday | ||
} | ||
var source = { | ||
key: tuesday | ||
} | ||
|
||
var expected = { | ||
key: tuesday | ||
} | ||
var actual = merge(target, source, {clone: true}) | ||
|
||
t.equal(actual.key, tuesday) | ||
t.end() | ||
}) | ||
|
||
test('should work on array with null in it', function(t) { | ||
var target = [] | ||
|
||
|
@@ -263,6 +414,18 @@ test('should work on array with null in it', function(t) { | |
t.end() | ||
}) | ||
|
||
test('should clone array\'s element if it is object', function(t) { | ||
var a = { key: 'yup' } | ||
var target = [] | ||
var source = [a] | ||
var expected = [{key: 'yup'}] | ||
|
||
var output = merge(target, source, {clone: true}) | ||
|
||
t.notEqual(output[0], a) | ||
t.equal(output[0].key, 'yup') | ||
t.end() | ||
}) | ||
test('should overwrite values when property is initialised but undefined', function(t) { | ||
var target1 = { value: [] } | ||
var target2 = { value: null } | ||
|
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.
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 think
e
needs to be cloned in this case, too - I think a failing test for this case might beBut I haven't tested that to make sure it fails right now.
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.
@TehShrike Good analytical thinking.