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 listener for file share #546

Merged

Conversation

daniel-beard
Copy link
Contributor

@daniel-beard daniel-beard commented Dec 4, 2018

Summary

Implements a robot.fileShared(res) -> listener. I believe this addresses #397

@daniel-beard daniel-beard changed the title Dbeard add listener for file share Add listener for file share Dec 4, 2018
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #546 into master will increase coverage by 0.65%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   85.63%   86.29%   +0.65%     
==========================================
  Files           6        6              
  Lines         355      372      +17     
  Branches       80       83       +3     
==========================================
+ Hits          304      321      +17     
  Misses         29       29              
  Partials       22       22
Impacted Files Coverage Δ
src/extensions.coffee 76.47% <100%> (+8.47%) ⬆️
src/message.coffee 90.42% <100%> (+0.31%) ⬆️
src/client.coffee 95.14% <100%> (+0.04%) ⬆️
src/bot.coffee 74.74% <80%> (+1.06%) ⬆️

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 571803a...4d93e35. Read the comment docs.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 14, 2018

this looks really good! thanks so much for contributing and being so thorough.

i feel pretty good about your implementation and merging it. i just want to acknowledge that this will be one more class method that we need to make sure is working properly since the issues in #541 were discovered.

@daniel-beard have you tried looking into the test coverage issues? it seems like the code inside eventHandler isn't being covered, even though you have a test that seems to call it. if you need a hand i can try taking a look at that too.

@daniel-beard
Copy link
Contributor Author

Thanks for the tip @aoberoi, tracked down which parts I was missing some tests for and added them :)

@shaydewael
Copy link
Contributor

shaydewael commented Dec 19, 2018

Hey @daniel-beard 👋these tests look great so far, thanks for adding them. I just merged a PR (#541) that tests for the extension methods (react and presenceChange right now) and I'd like to test for this one as well before we approve and merge. This could be added to the existing test.

I'm happy to resolve the merge conflict I just created and add the additional test if you want, or I can let you handle it if you'd like.

@shaydewael
Copy link
Contributor

shaydewael commented Dec 20, 2018

@daniel-beard I went ahead and resolved the merge conflicts and fixed the tests based on #541. Hoping to have this merged and in a release by end of day 😃

Copy link
Contributor

@shaydewael shaydewael left a comment

Choose a reason for hiding this comment

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

One small change, but happy to approve and merge once it's fixed 😄

test/bot.coffee Outdated Show resolved Hide resolved
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