-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Exclude development dependency from public RSpec config #258
Exclude development dependency from public RSpec config #258
Conversation
See following GitHub issue for more details slack-ruby#257
06bbae6
to
aab7ee8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, thank you! Also please update UPGRADING.md. I suspect some changes will have to be made by dependent projects, such as https://github.com/slack-ruby/slack-ruby-bot-server, and bots, e.g. https://github.com/dblock/slack-strava
CONTRIBUTING.md
Outdated
@@ -18,13 +18,27 @@ git remote add upstream https://github.com/slack-ruby/slack-ruby-bot.git | |||
|
|||
## Bundle Install and Test | |||
|
|||
Ensure that you can build the project and run tests. | |||
First make sure you've installed all default gems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CONTRIBUTING.md
Outdated
To run the full specs suite you'll need to install one of the optional gems: `async-websocket`, `faye-websocket` or `celluloid-io`. | ||
|
||
``` | ||
gem install async-websocket -v 0.8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to recommend a global async-websocket install, it should go into the Gemfile of the application using the library (which is already in the docs). The correct(er) way is to export CONCURRENCY=async-websocket
before running bundle install
, and it will install the network lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to put here.
I wouldn't rely on other app Gemfile while developing this gem since you might not have such app at all in your dev environment. The instruction to install the async-websocket
is not a recommendation but an example of a minimum setup to run the specs. Do you want to highlight that it's just an example? Or you have something more specific in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is a CONTRIBUTING to this project, and our Gemfile includes this. Remove this paragraph and say to export CONCURRENCY=async-websocket
before bundle install
and running tests.
CONTRIBUTING.md
Outdated
bundle exec rake CONCURRENCY=async-websocket | ||
``` | ||
|
||
Take a look at travis configuration for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to reference Travis, link .travis.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
aab7ee8
to
9619e20
Compare
@dblock I've updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I still have some minor nitpicks if it's ok?
CONTRIBUTING.md
Outdated
Run specs with: | ||
|
||
``` | ||
bundle exec rake CONCURRENCY=async-websocket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we export
concurrency, there won't be a need to set it here.
UPGRADING.md
Outdated
|
||
#### Set up VCR explicitly | ||
|
||
Requiring `slack-ruby-bot/rspec` won't set up [VCR](https://rubygems.org/gems/vcr) anymore. If your spec suite implicitly relies on this you would need to set up VCR explicitly in your spec suite. Just follow standard VCR documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't -> will no longer
Version should be 0.15.0, since it's a backwards incompatible change. Bump in version.rb, too, and CHANGELOG.
UPGRADING.md
Outdated
|
||
Requiring `slack-ruby-bot/rspec` won't set up [VCR](https://rubygems.org/gems/vcr) anymore. If your spec suite implicitly relies on this you would need to set up VCR explicitly in your spec suite. Just follow standard VCR documentation. | ||
|
||
See issue [#257](https://github.com/slack-ruby/slack-ruby-bot/issues/257) for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the word "issue" like below. I'm OCD.
@dblock I've updated the |
Thank you, merged. |
Closes #257
I can confirm that it fixes the problem I described in the linked issue.