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

Handle received data asynchronously #93

Merged
merged 4 commits into from
May 21, 2016
Merged

Conversation

sarahzrf
Copy link
Contributor

@sarahzrf sarahzrf commented May 20, 2016

See #92.

@dblock
Copy link
Collaborator

dblock commented May 20, 2016

This looks good. Can you please update CHANGELOG. Could use a test maybe too?

@dblock
Copy link
Collaborator

dblock commented May 20, 2016

The build is breaking for some other reason (something wrong with vcr, possibly just need to lock down that one to a previous release - can you please check it out - I am happy to fix if it's too difficult).

@sarahzrf
Copy link
Contributor Author

What kind of test could I write that wouldn't be extremely nondeterministic?

@dblock
Copy link
Collaborator

dblock commented May 20, 2016

I would just make sure that the function implemented by the bot command is invoked via async, so somewhere I'd have something like expect(...).to receive(:async).and_.... But if it's too complicated it's fine without it.

@sarahzrf
Copy link
Contributor Author

Oh, I could probably have the first command sleep indefinitely for the test. I'll try implementing that soon.

@sarahzrf
Copy link
Contributor Author

OK, it looks like it'd be fairly complicated to write a test. I might be able to figure something out, but it'd definitely take a lot of digging and learning about how the library and its tests are set up.

@dblock
Copy link
Collaborator

dblock commented May 20, 2016

This looks good. Could you please try to fix the build or let me know if you can't?

@sarahzrf
Copy link
Contributor Author

It looks like the issue is with vcr - there was a new minor version released today.

@sarahzrf
Copy link
Contributor Author

vcr/vcr#582

@dblock
Copy link
Collaborator

dblock commented May 21, 2016

Lock vcr to 3.0.1 in this PR?

@sarahzrf
Copy link
Contributor Author

sarahzrf commented May 21, 2016

ok - but it seems like kind of an inappropriate place to do it.

@sarahzrf
Copy link
Contributor Author

The issue on the vcr repo suggests that this should fix it too

@sarahzrf
Copy link
Contributor Author

ok nvm, I'll just fix the vcr version.

@dblock
Copy link
Collaborator

dblock commented May 21, 2016

Ha, it's now failing with another issue around bundler. That's rubygems/bundler#3558 and the fix is to update bundler:

before_install:
   - gem update bundler

Please. I just want this build to be green.

@sarahzrf
Copy link
Contributor Author

sarahzrf commented May 21, 2016

Wait - would you prefer that I undo the last commit and just lock the vcr version, or add bundler update instead?

@dblock
Copy link
Collaborator

dblock commented May 21, 2016

Honestly I don't have a preference. It's just mechanics.

@sarahzrf
Copy link
Contributor Author

The build is passing

@dblock dblock merged commit 2507790 into slack-ruby:master May 21, 2016
@dblock
Copy link
Collaborator

dblock commented May 21, 2016

Merged, thanks.

@sarahzrf sarahzrf deleted the patch-1 branch May 22, 2016 00:00
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.

2 participants