-
-
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
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
undefined
Not 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See npm/toArray for an example of using a helper function like mapToArray.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Ah, got it. Thank you.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_.keys
method. Happy to use a loop though if it is deemed the better option.