jessetane / queue

Asynchronous function queue with adjustable concurrency
MIT License
762 stars 66 forks source link

added an example for passing a result to the success handler #64

Closed dcsim0n closed 5 years ago

dcsim0n commented 5 years ago

Here is a suggestion for some additional documentation. I had trouble figuring this out through trial and error.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 369c5a0852f0a895b67d3056c84aedf5a179e831 on dcsim0n:callback_result_example into 0b663a830e4b35509b800fe4c37e17c1044d529d on jessetane:master.

jessetane commented 5 years ago

Thanks for the patch - the example in the readme is (or at least should be) an exact copy of the code here: https://github.com/jessetane/queue/blob/master/example/index.js

Ideally, I'd like to keep this executable, and mirror what's in the example program, can you update this pr accordingly?

dcsim0n commented 5 years ago

Sure thing, stand by.

dcsim0n commented 5 years ago

Ok, updated the example index.js file npm run example executes as expected and shows the proper result when appropriate, fixed some of my style errors.

jessetane commented 5 years ago

thanks, now that i can run the example this is a little easier for me to understand.

having implemented this: https://github.com/jessetane/queue/commit/ee1afdc30d5c3ae78e7157c15c39d269f5eb7dd1 https://github.com/jessetane/queue/blob/master/test/results.js

and answered related questions here: https://github.com/jessetane/queue/issues?utf8=%E2%9C%93&q=results

I wonder if we're documenting this in a way that's more confusing than it should be... if you're willing, it would be awesome to know if you had considered using opts.results but still found accessing results via the success handler to be superior and/or particularly important for some use case?

jessetane commented 5 years ago

basically, I wrote the example before the opts.results feature existed - so just wondering if we should be emphasizing that more instead, or if the success handler way is important in its own right

dcsim0n commented 5 years ago

Honestly, I didn’t look at the tests. But I understand more about how the opts.results array was intended to be used now after I have.

For me the results had to be handled in different ways using a switch based on the type of data received. Using the opts.results array I could iterate and pass each result to the switch. But the success handler seems to eliminate that step.

I know how much time writing docs can take, and just thought I’d make a suggestion based on something I had trouble with. Otherwise this module has worked very well for me and saved a lot of time by being simple to implement and robust.

jessetane commented 5 years ago

Heard. It would be nice to have both ways documented.. I can't work on it today but soon I'll merge this and add a commit to change the example over to use opts.results. I think we can get something that remains minimal but covers both techniques. Thanks again for your help.

dcsim0n commented 5 years ago

Heard. It would be nice to have both ways documented.. I can't work on it today but soon I'll merge this and add a commit to change the example over to use opts.results. I think we can get something that remains minimal but covers both techniques. Thanks again for your help.

Now that I understand how you intended opts.results to work, I could write another commit for that if you like.

jessetane commented 5 years ago

oh, feel free. for the success handler, is it as simple as just adding the result to the existing console.log?

https://github.com/jessetane/queue/blob/master/example/index.js#L75

dcsim0n commented 5 years ago

Yes, adding the result argument to the log would work. So long as you don't mind printing undefined for all the other jobs that don't pass a result.

dcsim0n commented 5 years ago

Alright, take a look at the latest commit and let me know what you think.

jessetane commented 5 years ago

might make a couple small tweaks but generally lgtm. sorry for the delay, thanks for the help!