Skip to content
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

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

Merged
merged 3 commits into from
Jun 18, 2019

Conversation

dcsim0n
Copy link
Contributor

@dcsim0n dcsim0n commented Jun 10, 2019

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

@coveralls
Copy link

coveralls commented Jun 10, 2019

Coverage Status

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

@jessetane
Copy link
Owner

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
Copy link
Contributor Author

dcsim0n commented Jun 10, 2019

Sure thing, stand by.

included an example of passing the result of a resolved promise
to the 'success' event listener. Also shows how to pass an error
from a rejected promise. Fixed some style errors.
@dcsim0n
Copy link
Contributor Author

dcsim0n commented Jun 10, 2019

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
Copy link
Owner

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

having implemented this:
ee1afdc
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
Copy link
Owner

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
Copy link
Contributor Author

dcsim0n commented Jun 11, 2019

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
Copy link
Owner

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.

asyncJob()
.then((result) => cb(null, result))
.catch((error) => cb(error))
})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may be able to do q.push(asyncJob) here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, you could just return a promise. If we want to demonstrate how to pass a result into a callback() then maybe a different approach would work for you.

Suggested change
})
const result = 'this is a long stream of data'
cb(null, result)
})

@dcsim0n
Copy link
Contributor Author

dcsim0n commented Jun 12, 2019

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
Copy link
Owner

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
Copy link
Contributor Author

dcsim0n commented Jun 12, 2019

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.

incorporated examples of how to use the results array
@dcsim0n
Copy link
Contributor Author

dcsim0n commented Jun 15, 2019

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

@jessetane jessetane merged commit 691f70e into jessetane:master Jun 18, 2019
@jessetane
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants