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

Don't monkey-patch SlackRubyBot::Client #15

Open
dblock opened this issue Jun 2, 2016 · 4 comments
Open

Don't monkey-patch SlackRubyBot::Client #15

dblock opened this issue Jun 2, 2016 · 4 comments
Labels

Comments

@dblock
Copy link
Collaborator

dblock commented Jun 2, 2016

https://github.com/dblock/slack-ruby-bot-server/blob/master/lib/slack-ruby-bot-server/ext/slack-ruby-bot/client.rb contains a monkey patch that adds a client field to SlackRubyBot::Client. That shouldn't be necessary, the client instantiated should be a child of SlackRubyBot::Client.

@dblock dblock added the chore label Jun 2, 2016
@dblock dblock changed the title Use inheritance and avoid monkey-patching SlackRubyBot::Client Don't monkey-patch SlackRubyBot::Client Jun 2, 2016
@tmsrjs
Copy link
Collaborator

tmsrjs commented Jun 2, 2016

It seems that SlackRubyBot::Client#owner is used inside the sample_app to log team info (team_id, name and domain), but this info is already present in SlackRubyBot::Client#team.
I think we can safely remove the monkey patch and modify the sample_app to pull this info from SlackRubyBot::Client#team.
What do you think?

@dblock
Copy link
Collaborator Author

dblock commented Jun 2, 2016

Possibly, but in most projects you need the actual stored team record with the token (the owner), not info about it after the client boots (which is what is stored as team), or some way to retrieve it, especially on restart. Check out how it's used in say https://github.com/dblock/slack-market.

@tmsrjs
Copy link
Collaborator

tmsrjs commented Jun 2, 2016

Maybe I'm missing something, but couldn't we just Team.find_by(team_id: client.team.id) lazily when needed? The only scenario I think this could be a problem is if the same mongodb is used to store info for more than one slack-ruby-bot-server and more than one of those bots is added to the same team, but that sounds like bad design anyway.

@dblock
Copy link
Collaborator Author

dblock commented Jun 2, 2016

Isn't there a chicken-and-egg problem of knowing client.team.id? I am probably wrong, so as long as we can make it work, amen :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants