-
-
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
Use config.token and config.aliases if set #254
Conversation
it 'sets config.token from ENV' do | ||
allow(ENV).to receive(:[]).and_call_original.at_least(:once) | ||
allow(ENV).to receive(:[]).with('SLACK_API_TOKEN').and_return(token) | ||
SlackRubyBot.configure { |config| config.token = nil } |
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 pattern deviates from the existing, but I do feel it makes the tests more isolated from the setup in spec_helper. What do you think @dblock?
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 like it.
Let's also allow passing the token in the constructor for |
|
lib/slack-ruby-bot/rspec/support/slack-ruby-bot/it_behaves_like_a_slack_bot.rb
Outdated
Show resolved
Hide resolved
It's more of a best practice thing, we can have a singleton, but not globals where it's unnecessary. I don't have a better scenario, but for testing alone it's worth it IMHO.
We possibly have issues here, but I am not sure. The thing is that most people use this library with slack-ruby-bot-server, that doesn't use any of these globals and creates a bot instance with a token following an OAuth workflow. The globals were a facility to run a single bot for a single team with a token, which is deprecated by slack. Feel free to fix all/some/none! And greatly appreciate it and happy to help. |
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 great. A few more asks, thanks for hanging in here with me.
Fixes #240.
I assumed that setting the config in code would take precedence over an ENV variable.
Will do a separate PR for aliases after this!