Add support for maps in _.pairs method ...Closes #2517 #2523
Conversation
add support for maps within _.pairs method by returning array of map entries
|
Looking into failed tests... |
|
Should this support Sets too? |
|
What would be the desired behavior for |
|
I'm not entirely sure. It could be the same entries call, with the output |
penDerGraft
changed the title
|
Using the logic in the last commit, if a check for var s = new Set([1,2,3]);
_.pairs(s);
// => [[1,1], [2,2], [3,3]]@akre54 If that's what you're envisioning, I can add the check. |
|
Yeah I think it should be treated like an object with |
|
Ok, as part of this PR or a new one? Seems like it could be a separate issue. |
| pairs[i] = [keys[i], obj[keys[i]]]; | ||
| var pairs, i, length; | ||
| if (_.isMap(obj)) { | ||
| var entries = obj.entries(); |
There was a problem hiding this comment.
Why'd you remove the Array.from here? Seems like if you had Map you'd also have Array.from.
There was a problem hiding this comment.
I'm really not a fan of this much branching logic for Map (which I think you probably wouldn't even use Underscore with in the first place).
There was a problem hiding this comment.
Yeah, I agree that Array.from is a cleaner solution. Unfortunately it isn't supported in node 0.12.*, hence the failed tests.
node v0.12.13 below:
> typeof Map
'function'
> Array.from
undefinedNot sure on the question of whether or not to even use underscore with Maps. I saw #2517 and decided to submit a PR.
There was a problem hiding this comment.
What's the first version it was supported in? We could also drop support
for node 0.12.x since it's pretty ancient by this point
On Mon, May 2, 2016, 18:44 Brent Pendergraft notifications@github.com
wrote:
In underscore.js
#2523 (comment):@@ -1009,11 +1009,21 @@
// Convert an object into a list of
[key, value]pairs.
_.pairs = function(obj) {
- var keys = _.keys(obj);
- var length = keys.length;
- var pairs = Array(length);
- for (var i = 0; i < length; i++) {
pairs[i] = [keys[i], obj[keys[i]]];- var pairs, i, length;
- if (_.isMap(obj)) {
var entries = obj.entries();Yeah, I agree that Array.from is a cleaner solution. Unfortunately it
isn't supported in node 0.12.*, hence the failed tests.typeof Map'function'> Array.fromundefined
Not sure on the question of whether or not to even use underscore with
Maps. I saw #2517
#2517 (comment)
and decided to submit a PR.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/underscore/pull/2523/files/742033f3de979e46a99d3e9553088e5897d49952#r61817716
Adam K (mobile)
There was a problem hiding this comment.
Looks like 4.0.0, which I think was the next official node.js release. Everything between the two was io.js.
There was a problem hiding this comment.
Thanks for the tip. I had thought about that, but wasn't sure if it would cause confusion since Array#forEach is handled by _.each. So, _.toArray would handle Maps and _.pairs would use _.toArray?
There was a problem hiding this comment.
See npm/toArray for an example of using a helper function like mapToArray.
There was a problem hiding this comment.
Sorry, just want to make sure I'm on the same page. So are we looking for a separate helper function here, or just the same _.pairs function being used in other places?
There was a problem hiding this comment.
A helper function to implement both. I'm showing that the code cost is spread around because it can be used for more than instance.
|
Separate pull is fine, interested to hear the other maintainers thoughts on it too. |
| @@ -82,7 +82,10 @@ | |||
| assert.deepEqual(_.pairs({one: 1, two: 2}), [['one', 1], ['two', 2]], 'can convert an object into pairs'); | |||
| assert.deepEqual(_.pairs({one: 1, two: 2, length: 3}), [['one', 1], ['two', 2], ['length', 3]], '... even when one of them is "length"'); | |||
| if (typeof Map === 'function') { | |||
| assert.deepEqual(_.pairs(new Map([['one', 1], ['two', 2]])), [['one', 1], ['two', 2]], '... or when object is a Map'); | |||
There was a problem hiding this comment.
I read in a previous PR that IE11 does not support initializing Maps with arrays in the constructor, hence the refactor here.
_add support for maps within .pairs method by returning array of map entries.
I decided to go with a different approach. Returning an array of
obj.entries()seemed a little bit cleaner than iterating over each entry, and seemed to follow the pattern of type-check -> return in the_.keysmethod. Happy to use a loop though if it is deemed the better option.