slack-ruby-bot-server icon indicating copy to clipboard operation
slack-ruby-bot-server copied to clipboard

Prevent name collision for teams

Open leechou opened this issue 7 years ago • 12 comments

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

leechou avatar Nov 17 '18 00:11 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

leechou avatar Nov 19 '18 04:11 leechou

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".

dblock avatar Nov 19 '18 04:11 dblock

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?

leechou avatar Nov 19 '18 07:11 leechou

  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.

dblock avatar Nov 19 '18 13:11 dblock

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

leechou avatar Nov 20 '18 00:11 leechou

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'  }

dblock avatar Nov 20 '18 13:11 dblock

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.

leechou avatar Nov 20 '18 14:11 leechou

that LGTM

dblock avatar Nov 20 '18 16:11 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 avatar Nov 21 '18 19:11 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.

leechou avatar Nov 26 '18 07:11 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.

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 avatar Nov 26 '18 14:11 dblock

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

dblock avatar Apr 22 '19 13:04 dblock