Handle models declared inside modules out of the box #136

Closed
grzesiek opened this Issue Apr 29, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@grzesiek
Contributor

grzesiek commented Apr 29, 2015

Hi.

At this moment nobrainer handles namespaced models (inside modules) in inconvenient way.

For example - if you want to create associations you need to specify explicitly class_name because nobrainer uses constantize method from ActiveSupport::CoreExt. ActiveSupport's Inflector is looking for classes' names only in top level (ref https://github.com/rails/rails/blob/master/activesupport/lib/active_support/inflector/methods.rb#L250), so only specifying class name explicitly will work even if we want to create association to other class declared inside same module.

Similar issue is associated with name of table, this probably refers to

table_name.try(:to_s) || root_class.name.tableize.gsub('/', '__')
.

Using models inside ruby modules is quite frequent and, in my opinion, this should be handled out of the box.

What do you think ?

Cheers,
Grzesiek

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 29, 2015

Owner

Can you be more specific? If you could show a code example, and explain what nobrainer should do, that would be helpful

Owner

nviennot commented Apr 29, 2015

Can you be more specific? If you could show a code example, and explain what nobrainer should do, that would be helpful

@grzesiek

This comment has been minimized.

Show comment
Hide comment
@grzesiek

grzesiek Apr 29, 2015

Contributor

Suppose I have something like this:

class Project::Modules::Model
  include NoBrainer::Document

  store_in database: -> { User.current }, table: 'models'
  belongs_to :user, class_name: Project::Modules::User.name
end

This works, but I need to specify class_name explicitly (this raises NameError if it is not). I also need to specify table name, because nobrainer would create table name like project__modules__model.

This is somewhat inconvenient as these two models (User and Model) are declared in same module.

Contributor

grzesiek commented Apr 29, 2015

Suppose I have something like this:

class Project::Modules::Model
  include NoBrainer::Document

  store_in database: -> { User.current }, table: 'models'
  belongs_to :user, class_name: Project::Modules::User.name
end

This works, but I need to specify class_name explicitly (this raises NameError if it is not). I also need to specify table name, because nobrainer would create table name like project__modules__model.

This is somewhat inconvenient as these two models (User and Model) are declared in same module.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 29, 2015

Owner
  1. The association classes can be looked up in the current module first, that makes sense.
  2. About the table name, it's a bit difficult to only use the class name, because if you have two classes, ProjectA::User, and ProjectB::User, we don't want to have table conflicts.
Owner

nviennot commented Apr 29, 2015

  1. The association classes can be looked up in the current module first, that makes sense.
  2. About the table name, it's a bit difficult to only use the class name, because if you have two classes, ProjectA::User, and ProjectB::User, we don't want to have table conflicts.
@grzesiek

This comment has been minimized.

Show comment
Hide comment
@grzesiek

grzesiek Apr 29, 2015

Contributor
  1. That makes sense, and I believe that this would be easy to implement, do you want me to try to do so ?
  2. Yes, I agree that this shouldn't be default behavior, however achieving that using some configuration variable (like config.implicit_short_table_names) would also make sense for me.
Contributor

grzesiek commented Apr 29, 2015

  1. That makes sense, and I believe that this would be easy to implement, do you want me to try to do so ?
  2. Yes, I agree that this shouldn't be default behavior, however achieving that using some configuration variable (like config.implicit_short_table_names) would also make sense for me.
@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 30, 2015

Owner
  1. yes!
  2. This would not be so flexible, full control is desired (e.g. one might want to add a prefix or suffix on all table names). You can put this in an initializer to achieve the desired effect:
module NoBrainer::Document::ClassMethods
  def table_name
    super.gsub(/^project__modules__model/, '')
  end
end
Owner

nviennot commented Apr 30, 2015

  1. yes!
  2. This would not be so flexible, full control is desired (e.g. one might want to add a prefix or suffix on all table names). You can put this in an initializer to achieve the desired effect:
module NoBrainer::Document::ClassMethods
  def table_name
    super.gsub(/^project__modules__model/, '')
  end
end
@grzesiek

This comment has been minimized.

Show comment
Hide comment
@grzesiek

grzesiek Apr 30, 2015

Contributor
  1. Check PR above. Frankly I don't have much spare time at the moment, so this probably needs some more second thoughts/investigaion.
Contributor

grzesiek commented Apr 30, 2015

  1. Check PR above. Frankly I don't have much spare time at the moment, so this probably needs some more second thoughts/investigaion.

@nviennot nviennot closed this in b904b61 May 2, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot May 2, 2015

Owner

I've amended your PR, and pushed it. Let me know if it fits your needs :)

Nico

Owner

nviennot commented May 2, 2015

I've amended your PR, and pushed it. Let me know if it fits your needs :)

Nico

@grzesiek

This comment has been minimized.

Show comment
Hide comment
@grzesiek

grzesiek May 6, 2015

Contributor

👍 It works for me, thanks !

Contributor

grzesiek commented May 6, 2015

👍 It works for me, thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment