-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Mapping over Objects #1177
Mapping over Objects #1177
Conversation
async.mapValuesLimit(obj, 2, function (val, key, next) { | ||
running++; | ||
async.setImmediate(function () { | ||
expect(running).to.equal(concurrency[key]); |
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.
BTW, I'm experimenting with this way of verifying concurrency. It should be more reliable than asserting a concurrency after some timeout value. Could help with some of our flaky tests.
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.
Cool, let me know if you want testingbot
keys to try running in selenium
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.
We'd have to refactor other tests to be like this style, tracking concurrency and verifying things finish in a certain order, but it might be worth it 😓
eachfn(arr, function (value, index, callback) { | ||
iteratee(value, function (err, v) { | ||
results[index] = v; | ||
var idx = keys ? indexOf(keys, index, 0) : 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.
this shouldn't be necessary. Just keep track of by creating and incrementing a numeric index variable (idx
in the eachfn
var idx = 0;
eachfn(arr, function(...) {
idx += 1;
iteratee(...
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.
You're right. Fixed in 611a442
var assert = require('assert'); | ||
|
||
describe('mapValues', function () { | ||
var obj = {a: 1, b: 2, c: 3}; |
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.
Worth testing with an array as well?
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 would the expected behavior be?
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.
{0: <val>, 1: <val>, 2: <val>}
I would also be fine with leaving this behaviour implied
LGTM /cc me when you feel done with this PR and I'll review again+merge |
It's ready to merge as-is. I could add |
Mapping over Objects
I thanks think of plenty. For one, an object containing urls. async.map({
resource1: 'https://resource1.url',
resource2: 'https://resource1.url'
}, (url, cb) => fetchResource(url, cb), (err, results) => { ... }); Update: If anyone is curious about how to continue mapping objects, there is mapValues instead. |
WIP. Closes #1157.
I might add
mapKeys
and family later, but I can't contrive an example use-case where you'd need to map the keys of an object asynchronously.