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(extend): optionally deep-extend when last parameter is `true` #10519

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@caitp
Contributor

caitp commented Dec 18, 2014

Same as #10507 but with my review comments addressed and jscs fixed.

Someone quickly review this so we can land it :<

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Dec 18, 2014

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot commented Dec 18, 2014

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added the cla: no label Dec 18, 2014

Show outdated Hide outdated src/Angular.js
@@ -333,7 +333,7 @@ function setHashKey(obj, h) {
* Extends the destination object `dst` by copying own enumerable properties from the `src` object(s)
* to `dst`. You can specify multiple `src` objects. If you want to preserve original objects, you can do so
* by passing an empty object as the target: `var object = angular.extend({}, object1, object2)`.
* Note: Keep in mind that `angular.extend` does not support recursive merge (deep copy).
* Note: Pass `true` in as the last argument to perform a recursive merge (deep copy).

This comment has been minimized.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

We ought to have a @param tag for this but I am not sure how that fits with the variable argument src.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

We ought to have a @param tag for this but I am not sure how that fits with the variable argument src.

This comment has been minimized.

@caitp

caitp Dec 18, 2014

Contributor

yeah, it's not super clear how that would work... Maybe you could hack dgeni to process arguments that end with ... (or begin with ...) as a spread of parameters or something

@caitp

caitp Dec 18, 2014

Contributor

yeah, it's not super clear how that would work... Maybe you could hack dgeni to process arguments that end with ... (or begin with ...) as a spread of parameters or something

This comment has been minimized.

@caitp

caitp Dec 18, 2014

Contributor

Bloody autocorrect renames "dgeni" to "degree" -.-

@caitp

caitp Dec 18, 2014

Contributor

Bloody autocorrect renames "dgeni" to "degree" -.-

Show outdated Hide outdated src/Angular.js
var isDeep = false;
if (argsLength >= 3) {
var maybeIsDeep = arguments[argsLength - 1];
// Secret code to use deep extend without adding hash keys to object properties!

This comment has been minimized.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

should this comment be:

// Secret code to use deep extend without adding hash keys to source object properties!
@petebacondarwin

petebacondarwin Dec 18, 2014

Member

should this comment be:

// Secret code to use deep extend without adding hash keys to source object properties!

This comment has been minimized.

@caitp

caitp Dec 18, 2014

Contributor

it would be added to the destination object, I'll clarify that

@caitp

caitp Dec 18, 2014

Contributor

it would be added to the destination object, I'll clarify that

Show outdated Hide outdated src/Angular.js
if (isDeep && isObject(src)) {
if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {};
extend(dst[key], src, 0xFACECAFE);

This comment has been minimized.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

What about object cycles? At least, I think we should put something in the docs about not trying to deep extend an object that has cyclic references.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

What about object cycles? At least, I think we should put something in the docs about not trying to deep extend an object that has cyclic references.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Dec 18, 2014

Member

there is something odd in having an optional parameter after a variable number of parameters... why not put this optional parameter second?

Member

lgalfaso commented Dec 18, 2014

there is something odd in having an optional parameter after a variable number of parameters... why not put this optional parameter second?

Show outdated Hide outdated test/AngularSpec.js
});
it('should create an empty array if destination property is a non-object and source property is an array when deep-extending', function() {

This comment has been minimized.

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

'should create a new array ...'?

@petebacondarwin

petebacondarwin Dec 18, 2014

Member

'should create a new array ...'?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 18, 2014

Member

Apart from the minor docs/comments notes I made this looks good to me.

Member

petebacondarwin commented Dec 18, 2014

Apart from the minor docs/comments notes I made this looks good to me.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 18, 2014

Contributor

there is something odd in having an optional parameter after a variable number of parameters... why not put this optional parameter second?

It would be lovely to do it that way, but it would be a breaking change... This is what I didn't like about the PR in the first place, but it doesn't seem "that bad" really

Contributor

caitp commented Dec 18, 2014

there is something odd in having an optional parameter after a variable number of parameters... why not put this optional parameter second?

It would be lovely to do it that way, but it would be a breaking change... This is what I didn't like about the PR in the first place, but it doesn't seem "that bad" really

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Dec 18, 2014

Member

Why would it be a breaking change?

there is something odd in having an optional parameter after a variable
number of parameters... why not put this optional parameter second?

It would be lovely to do it that way, but it would be a breaking change...
This is what I didn't like about the PR in the first place, but it doesn't
seem "that bad" really


Reply to this email directly or view it on GitHub
#10519 (comment).

Member

lgalfaso commented Dec 18, 2014

Why would it be a breaking change?

there is something odd in having an optional parameter after a variable
number of parameters... why not put this optional parameter second?

It would be lovely to do it that way, but it would be a breaking change...
This is what I didn't like about the PR in the first place, but it doesn't
seem "that bad" really


Reply to this email directly or view it on GitHub
#10519 (comment).

@petebacondarwin petebacondarwin modified the milestone: 1.3.8 Dec 18, 2014

Show outdated Hide outdated src/Angular.js
}
}
}
setHashKey(dst, h);
if (isDeep !== 0xFACECAFE) setHashKey(dst, h);

This comment has been minimized.

@gkalpak

gkalpak Dec 19, 2014

Member

We should probably still attach a hashkey if h is defined.

@gkalpak

gkalpak Dec 19, 2014

Member

We should probably still attach a hashkey if h is defined.

This comment has been minimized.

@caitp

caitp Dec 20, 2014

Contributor

yeah, I think that makes sense, will get rid of FACECAFE =)

@caitp

caitp Dec 20, 2014

Contributor

yeah, I think that makes sense, will get rid of FACECAFE =)

This comment has been minimized.

@OMarohn

OMarohn Dec 20, 2014

Ich bin bis 05.01.2015 abwesend.

Ich werde vertreten durch Herrn T. Landgraf und Herrn D. Neander.

Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re:
[angular.js] feat(extend): optionally deep-extend when last parameter is
true (#10519)" gesendet am 20.12.2014 03:58:31.

Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während
diese Person abwesend ist.=

@OMarohn

OMarohn Dec 20, 2014

Ich bin bis 05.01.2015 abwesend.

Ich werde vertreten durch Herrn T. Landgraf und Herrn D. Neander.

Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re:
[angular.js] feat(extend): optionally deep-extend when last parameter is
true (#10519)" gesendet am 20.12.2014 03:58:31.

Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während
diese Person abwesend ist.=

@btford btford modified the milestones: 1.3.8, 1.3.9 Dec 19, 2014

@gdi2290

This comment has been minimized.

Show comment
Hide comment
@gdi2290

gdi2290 Dec 20, 2014

Member

I rather have angular.extend and angular.merge (for deep-extend)

Member

gdi2290 commented Dec 20, 2014

I rather have angular.extend and angular.merge (for deep-extend)

feat(Angular.js): implement angular.merge() helper
angular.merge() is like angular.extend(), but will "deeply" copy object properties from source
objects.
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 20, 2014

Contributor

@gdi2290 I've implemented your suggestion --- It's slightly more code but it doesn't really make a big difference, so I'm fine with either one or neither

Contributor

caitp commented Dec 20, 2014

@gdi2290 I've implemented your suggestion --- It's slightly more code but it doesn't really make a big difference, so I'm fine with either one or neither

perf(extend/merge): explicitly ignore hashKey property
Previously, hashKeys were copied from source objects and subsequently deleted or replaced with the
original. Now, there is no need to delete properties (which can be expensive), and no need to track
the initial hashKey value.
@gdi2290

This comment has been minimized.

Show comment
Hide comment
@gdi2290

gdi2290 Dec 20, 2014

Member

@caitp thanks, in the merge tests you should probably remove the third argument

merge(dst, src, true);
Member

gdi2290 commented Dec 20, 2014

@caitp thanks, in the merge tests you should probably remove the third argument

merge(dst, src, true);
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 20, 2014

Contributor

heh missed a few --- but they won't matter anyway, it's good to have them even to verify it won't throw on primitives :>

Contributor

caitp commented Dec 20, 2014

heh missed a few --- but they won't matter anyway, it's good to have them even to verify it won't throw on primitives :>

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 3, 2015

Member

I'll tweak the tests and land this today.

Member

petebacondarwin commented Mar 3, 2015

I'll tweak the tests and land this today.

@petebacondarwin petebacondarwin self-assigned this Mar 3, 2015

@caitp caitp closed this in c0498d4 Mar 3, 2015

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment