suweller / mongoid-autoinc

⬆ Auto incrementing fields for Mongoid documents
MIT License
62 stars 44 forks source link

Adds support for sub-classes to use inherited class name. #12

Closed krismartin closed 10 years ago

matsimitsu commented 11 years ago

Sweet! :+1:

@suweller ?

mgartner commented 11 years ago

This probably only works for immediate parents, not grandparents or beyond, right? For example, the Lion class below would use "Mammal" as the name. Right?

class Animal
  include Mongoid::Autoinc

  increments :tracking_number
end

class Mammal < Animal
  include Mongoid::Autoinc

  increments :tracking_number, use_inherited_class_name: true
end

class Lion < Mammal
  include Mongoid::Autoic

  increments :tracking_number, use_inherited_class_name: true
end

Also I think the name of the option is a little confusing. Maybe something like use_parent_scope.

krismartin commented 11 years ago

Yes, that's correct. It only use the immediate parent model name.

I can change the code slightly to look at the last ancestor that respond to the method 'model_name'. So using your example, the class Lion would use "Animal" as the name. Would you like me to add this?

I'm happy with the option name as you suggested.

mgartner commented 11 years ago

I'm not sure what behavior is better, and ultimately it's not my decision. But whatever is decided I would suggest that the README makes this point clear.

suweller commented 11 years ago

I like the idea of this, but agree with @mgartner that it may not be a very good name. What do you guys think of using_parent_class.

When we add model_name a Mongoid::Document instance responds to the same method as its class, but with entirely different behavior. I'm not sure if this is something we want.

edit: I do like this feature a lot though, don't get me wrong. I just want to think about it a little more ;-)

johnnyshields commented 10 years ago

I've raised an alternate PR #15 which allows :model_name by itself to be specified as an option. I think this is ultimately more flexible as in my case I'm not using subclasses but namespaces (as part of a Rails Engines segmented architecture)

suweller commented 10 years ago

We've decided to go with @johnnyshields version of this feature. I'm sure this pull request has helped all of us to better find the solution best suited for us. Thanks, @krismartin! I'll add you to the list of contributors :-)

krismartin commented 10 years ago

Thank you guys. Good outcome!