Prevent name collision for teams

[leechou]

Prevents name collision for teams so that the gem can be used with other projects.

• Prevents collision for class Team. References to teams are now SlackRubyBotServer::Team
• Allow configuration of ActiveRecord table name. SlackRubyBotServer::Config.teams[:name]='my_table'

Minor bug fixes:

• Fixed spec for ActiveRecord
• Fixed console script

[dblock]

I like this and would like this to move forward. Thanks.

• I think the configuration option should be called store_teams_in (or better name?) and have downstream effect on both Mongoid and ActiveRecord. In MongoDB there are no "tables", but "collections".
• This is not backwards compatible and will break anyone with a Team class, I believe. I have half a dozen bots that extend the Team. Give it a try on anything like http://github.com/dblock/slack-strava and you'll see what I mean. At the very least we need UPGRADING to explain what one should do. But I think the right long term solution is that rather than namespacing the team class we want to turn it into a module that can be included in both Mongoid and AR scenarios. In which case we don't actually need to specify the table name, this would be the responsibility of the class using the module.

What do you think?

[leechou]

I feel like importing another module will add unnecessary complexity, and easy fix for those that subclass Teams is just to add Team = SlackRubyBot::Team and keep everything as-is.

As for the name of the variable to store the table name, I prefer SlackRubyBotServer::Config.teams_table = 'my_table' since it correspond to the thought of "for SlackRubyBotServer, configure the teams table to be 'my_table'"

Using something like store_teams_in would be more like objective-C style wording, like -(void)showAlertMsg:(NSString *)message withTitle:(NSString *)title; which would correspond to "show alert message 'my message' with title 'my title'"

I don't mind either way, it just makes more sense to me to have the variable as the noun of what it is, like teams_table or activerecord_teams_table

[dblock]

I don't mind either way, it just makes more sense to me to have the variable as the noun of what it is, like teams_table or activerecord_teams_table

Except that there're no "tables" in MongoDB, so it only makes sense for AR. In MongoDB these are called "collections".

[leechou]

So which would you prefer?

1. Change var name to teams_table for AR, and teams_collection for mongoid.
2. Change var name to teams_storage_name
3. Other name?

Did you want the mongoid patch before accepting this PR?

[dblock]

1. Change var name to teams_table for AR, and teams_collection for mongoid.
2. Change var name to teams_storage_name
3. Other name?

I think the cleanest and most forward-looking solution is a store_in that works for all storage, a hash where you can do:

.store_in = { name: 'foobar', other_options: ... }

This way future we can extend this in the future. What do you think?

Did you want the mongoid patch before accepting this PR?

I'd like Mongoid to work as part of this PR as well, but would accept without it.

[leechou]

so, .store_in = {teams: 'my_table_or_collection_name'} ?

[dblock]

so, .store_in = {teams: 'my_table_or_collection_name'} ?

What if we just dropped store_in and did:

teams: { name: 'my_table' }

This way it's extensible for the future, for example

teams: { name: 'whatever', class_name: 'Whatever'  }

[leechou]

Did you mean something like this?
SlackRubyBotServer::Config.teams = {name: 'my_table', class_name: 'MyClass'}

I'm fine with it this way, since it'll read off as "configure the teams to have name 'my_table'". Only potential issue with this is consistency, since this is the only place where it's configured like this.

Let me know about the final call and I'll make the updates.

[dblock]

that LGTM

[dblock]

This needs a major version bump.

If you have time, consider doing the MongoDB support for renaming the collection as part of this.

Try writing/upgrading a trivial bot. I personally only have MongoDB ones.

[dblock]

This should say SlackRubyBotServer not SlackRubyBot, right?

Also, I could be wrong, but I think this might not work for MongoDB because it might change the name of the collection to something like slack_ruby_bot_server_team, that needs to be tested, so I suggest you actually do the change for mongodb as part of this PR, too.

[leechou]

ya, my bad. I'll get that changed.

Sure, I can test out MongoDB, but it might take a while though.

[leechou]

Just noticed that more changes needs to be done for this feature, since the db:migrate doesn't pull in the setting of tablename for ActiveRecord. Will continue to work on this.

[dblock]

Just noticed that more changes needs to be done for this feature, since the db:migrate doesn't pull in the setting of tablename for ActiveRecord. Will continue to work on this.

I still think turning Team into an include-able module could be a good idea. It would avoid these problems, migrations, etc. Just thinking out loud.

[dblock]

Related #102, in which the user tried to inherit from ApplicationRecord instead of ActiveRecord::Base. Could be fixed by this too.