Prevent name collision for teams
Prevents name collision for teams so that the gem can be used with other projects.
- Prevents collision for class
Team. References to teams are nowSlackRubyBotServer::Team - Allow configuration of ActiveRecord table name.
SlackRubyBotServer::Config.teams[:name]='my_table'
Minor bug fixes:
- Fixed spec for ActiveRecord
- Fixed console script
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
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_tableoractiverecord_teams_table
Except that there're no "tables" in MongoDB, so it only makes sense for AR. In MongoDB these are called "collections".
So which would you prefer?
- Change var name to
teams_tablefor AR, andteams_collectionfor mongoid. - Change var name to
teams_storage_name - Other name?
Did you want the mongoid patch before accepting this PR?
- Change var name to
teams_tablefor AR, andteams_collectionfor mongoid.- Change var name to
teams_storage_name- 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.
so, .store_in = {teams: 'my_table_or_collection_name'} ?
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' }
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.
that LGTM
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.
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.
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.
Related #102, in which the user tried to inherit from ApplicationRecord instead of ActiveRecord::Base. Could be fixed by this too.