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

Add callback support to SlackBot::{send,reply} #540

Merged
merged 12 commits into from
Apr 24, 2020

Conversation

rtlechow
Copy link
Contributor

@rtlechow rtlechow commented Oct 15, 2018

Summary

Allows SlackBot::{send,reply} to receive an optional callback. While
this is not documented, if a callback is passed to Hubot's
Response::{send,reply}, Hubot will pass it around.

Shamelessly plagiarized from #440 because I'm in need of this
functionality. Fixed the tests to work with @stubs.

Requirements (place an x in each [ ])

Allows `SlackBot::{send,reply}` to receive an optional callback. While
this is not documented, if a callback is passed to Hubot's
`Response::{send,reply}`, Hubot will pass it around.

Shamelessly plagiarized from slackapi#440 because I'm in need of this
functionality. Fixed the tests to work with @stubs.
@CLAassistant
Copy link

CLAassistant commented Oct 15, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

Merging #540 into master will decrease coverage by 0.09%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   85.42%   85.32%   -0.10%     
==========================================
  Files           6        6              
  Lines         391      402      +11     
  Branches       84       88       +4     
==========================================
+ Hits          334      343       +9     
  Misses         34       34              
- Partials       23       25       +2     
Impacted Files Coverage Δ
src/bot.coffee 75.43% <84.61%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 150948c...a0c4cf4. Read the comment docs.

@rtlechow
Copy link
Contributor Author

BTW, if you sign the CLA prior to creating a PR (by following the link from the PR template), the status doesn't get checked. 🤓

package.json Outdated Show resolved Hide resolved
src/bot.coffee Outdated Show resolved Hide resolved
src/bot.coffee Outdated Show resolved Hide resolved
src/bot.coffee Outdated Show resolved Hide resolved
@aoberoi
Copy link
Contributor

aoberoi commented Oct 19, 2018

thanks for the contribution. i'm happy to merge this and gain a new contributor! there's just a couple comments that need to be addressed first.

@rtlechow
Copy link
Contributor Author

@aoberoi Finally got around to fixing it up. Any thoughts on improving coverage?

src/bot.coffee Outdated Show resolved Hide resolved
src/bot.coffee Outdated Show resolved Hide resolved
@aoberoi
Copy link
Contributor

aoberoi commented Dec 14, 2018

@rtlechow welcome back! thanks for making the updates.

It looks like the case that wasn't hit is when there's a function in the messages array (but not in the last position of the array). I'm not sure that's a realistic situation, but I appreciate the graceful handling of that. It's just that the tests aren't exercising that code, so I think adding a test case that does would get the coverage high enough to merge.

@seratch
Copy link
Member

seratch commented Apr 10, 2020

As @aoberoi already approved this PR before, it looks good to me, too. We took a bit long time after this PR was created. Just in case, I'll wait for responses from others until next week before merging this.

@seratch seratch merged commit 53f097a into slackapi:master Apr 24, 2020
@seratch seratch added the enhancement M-T: A feature request for new functionality label Apr 24, 2020
@seratch seratch modified the milestones: 4.8.0, 4.9.0 Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants