Provide proper attribute inheritance when subclassing Models #2

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

mje113 commented Dec 7, 2012

Currently, Couchbase::Model stores attributes (and views) in a class variable set to a mutable hash. This breaks the expected subclass attribute inheritance behavior that ActiveModel provides.

Example:

class A < Couchbase::Model
  attribute :one
end

class B < A
  attribute :two
end

B.attributes #=> {:one => nil, :two => nil}
A.attributes #=> {:one => nil, :two => nil}
B.attributes.object_id == A.attributes.object_id #=> true

This patch enables proper inheritance when ::Rails is present by using Class#class_attribute and detecting subclass additions to attribute:

class A < Couchbase::Model
  attribute :one
end

class B < A
  attribute :two
end

A.attributes #=> {:one => nil}
B.attributes #=> {:one => nil, :two => nil}
B.attributes.object_id != A.attributes.object_id #=> true
Allow for proper ActiveModel style attribute inheritance when subclas…
…sing Couchbase::Model subclasses, when using Rails.
couchbase-model.gemspec
@@ -23,4 +23,5 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rdiscount'
s.add_development_dependency 'yard'
s.add_development_dependency 'debugger'
+ s.add_development_dependency 'rails'
@avsej

avsej Dec 7, 2012

Member

I think it will be good to have also in case just active model available. I think doesn't really need rails here. maybe you can provide some fallback when rails isn't availble?

@mje113

mje113 Dec 7, 2012

Contributor

Yeah, makes sense, I think I was being lazy. It would also need active_support. Changes coming.

Contributor

mje113 commented Dec 10, 2012

Ok, I managed to rewrite the patch to not rely on class_attribute. It was actually breaking some non-active model related functionality anyway.

The main change with @@attributes & @@views is instead of tracking each sub-classes' attributes in their own key on the one class variable, each subclass dupes it's parent's class or instance variable version into it's own class-level instance variable.

couchbase-model.gemspec
@@ -1,7 +1,7 @@
# -*- encoding: utf-8 -*-
$:.push File.expand_path("../lib", __FILE__)
require "couchbase/model/version"
-
+require 'pry'
@avsej

avsej Dec 10, 2012

Member

Please don't bring unrelated dependencies

test/setup.rb
@@ -15,10 +15,12 @@
# limitations under the License.
#
+module Rails; end
+require 'active_model'
@avsej

avsej Dec 10, 2012

Member

Guess Rails and ActiveModel also unnecessary here.

Contributor

mje113 commented Dec 10, 2012

Thanks, missed some cleanup.

Member

avsej commented Dec 18, 2012

Hi Mike, sorry for delay. I've uploaded your patch into our master repository, which managed by gerrit code review tool. Could you register there and accept our CLA? After that I can merge your change. Github is just a mirror of that repository. Your patch located here:

http://review.couchbase.org/23392

Thanks for your contribution

Sergey Avseyev

@ingenthr ingenthr closed this Dec 19, 2012

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