Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implemented functionQueue as a wrapping adapter for queue #253

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

brianmaissy commented Mar 3, 2013

Addresses issue #35. functionQueue is implemented trivially in 4 lines; the rest of the pull request is a clone and adaptation of both the README and tests for queue.

Contributor

brianmaissy commented Mar 3, 2013

Well that was trivial. While implementing I came across a couple decisions. If I made the wrong ones anywhere, let me know:

  1. Naming: I decided functionQueue was the most descriptive, and therefore the best, name.
  2. Callbacks: Callbacks passed to the queue when adding functions are a little superfluous here. They might be useful when batch adding an array of functions to the queue, but mostly I left them in for the sake of uniformity in the API. Simply leaving them out, which most people will do, already works fine.
  3. README: I could have made a much shorter README addendum saying "works exactly like a queue, go see the API over there" but I decided it was better to repeat the entire thing, changing the word task to function and things like that when appropriate, and showing the exact same examples, but with functions. At the expense of making the README a little longer I figured being as clear as possible was always good in documentation like this.
  4. Test redundancy: As I said above, I simply copied and then adapted all the tests for queue. I figured this was the easiest way to make sure there were no side effects to the behavior of the queue other than the fact that it takes functions instead of tasks. We already have a set of tests to prove that the queue works properly, why not use them? However, now there is a lot of redundant code in the tests. Ideally some of the duplicate code could be factored out, but that applies equally to all of the tests as far as I can tell. Maybe in the future the tests should all be refactored by introducing a set of helper functions that would be useful everywhere (a fancier testing library might help in that regard) but I didn't think this pull request was the time or place.
Owner

caolan commented Mar 28, 2014

This seems like something that should be done with streams rather than async.queue

@caolan caolan closed this Mar 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment