-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Object-preserving map() function: mapValues #1953
Conversation
_.mapValues = function(obj, iteratee, context) { | ||
if (obj == null) return {}; | ||
// mapValues returns an object and thus it doesn't make any sense for collections | ||
if(obj.length === +obj.length) return _.map(obj,iteratee,context) |
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.
Remove this, we should always return an object by contract
In contrast to _.map it returns an object
You'll also need to test that case
@megawac what is the expected behavior if you use this method with an array? |
Try doing |
@megawac corrected - thanks :) |
results = {}, | ||
currentKey; | ||
for (var index = 0; index < length; index++) { | ||
currentKey = keys[index]; |
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.
var key = keys[index]
to follow current style + everything travis reports
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.
sry for being so ignorant to the coding style warnings.
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.
No worries LGTM after this last minor style change
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.
This looks to be a copy of _.map
's style, which uses currentKey
.
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.
It is a nearly 1:1 copy of _map
, so I thought keeping the index variable name might make sense ...
👍 |
deepEqual(_.mapValues(obj, function(val) { | ||
return val * 2; | ||
}), {'a': 2, 'b': 4}, 'simple objects'); | ||
|
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.
mind adding a test for _.each([null, void 0, 1, "abc", [], {}], (val) => deepEqual(_.mapValues(val, _.identity), {}))
Also, mind adding a test for inherited values from prototype?
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.
Also, mind adding a test for inherited values from prototype?
sth. like this?
var proto = function(){this.a = 1};
proto.prototype.b = 1;
var obj = new proto();
deepEqual(_.mapValues(obj, _.identity), {a: 1});
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.
Yep!, also mind adding {}
and []
to that empty list
I'm +1 for this if you mind squashing your commits @greenify. |
Of couse - sorry for being so noobish ... |
Closes #1869 |
This looks good to me. Go for it. |
Object-preserving map() function: mapValues
This adds the
mapValues
methods.It works exactly like
map
, but preserves the object.There are at least three issues about this function (#220 #1867 #1887), and as I also do this frequently it would be awesome to get this into underscore.