loadDocs.js: replace async.each with async.eachLimit, document rate-limiting

nprapps / interactive-template

A Node-based template for starting news apps and interactive pages

MIT License

62 stars 20 forks source link

loadDocs.js: replace async.each with async.eachLimit, document rate-limiting #14

Closed benlk closed 5 years ago

benlk commented 5 years ago

Changes

Adds rate-limiting code from solution pasted in https://github.com/nprapps/interactive-template/issues/12
Improves documentation for why and how the rate-limiting works

Questions

[ ] Should this be extended to loadSheets.js?

thomaswilburn commented 5 years ago

This is good. For future reference, it's pretty atypical to have long doc comments in this codebase--I'll update to condense a bit. For example, Caolan's docs for async are very good and easy to find, in case someone wants to look up what eachLimit() does.

benlk commented 5 years ago

Part of the reason I left a long comment there was because I didn't know that Caolan was the maintainer of async.

thomaswilburn commented 5 years ago

Sure, but you knew the module name, which means you can find it on NPM (and it's an extremely common dependency--a lot of Node users are going to run into it). In the same way, we don't document regular API calls or decorators in our Flask-based templates, or individual D3/Underscore calls in our graphic templates.

I'm not saying long comments have no place. But I'd prefer to save them for cases where the code is doing something tricky, not where it's a pretty standard task throttle.