Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inheritance of serializer inheriting the cache configuration #1249

Merged
merged 1 commit into from
Oct 7, 2015

Conversation

Rodrigora
Copy link
Contributor

Hi all!

This PR resolves #1219 declaring the cache related fields with class_attribute.

Please, check if the two tests cases are relevant to this change. I felt I was testing the class_attribute behavior 😕.

end

assert PostSerializer._cache_key == 'post'
assert inherited_serializer._cache_key == 'new-key'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Please change to assert_equal and I think this is good to go.

Do you think there's any value in testing all the class_attributes? I think there would be

     serializer.class_attribute :_cache
      serializer.class_attribute :_fragmented
      serializer.class_attribute :_cache_key
      serializer.class_attribute :_cache_only
      serializer.class_attribute :_cache_except
      serializer.class_attribute :_cache_options
      serializer.class_attribute :_cache_digest

I have some similar changes in bf4@a60ba9d#diff-5624f9cf35be4dc254412d979fd293c4R31 that could go along with or follow this:

+    class_attribute :_attributes, instance_writer: false
+    self._attributes ||= []
+    class_attribute :_attributes_keys, instance_writer: false, instance_reader: false
+    self._attributes_keys ||= {}
+    class_attribute :_type, instance_writer: false
#
   def self.inherited(base)
+      base._attributes = _attributes.dup
+      base._attributes_keys = _attributes_keys.dup
#
    def attributes
+      attributes = _attributes.dup

      attributes.each_with_object({}) do |name, hash|
+        if _fragmented
+          hash[name] = _fragmented.public_send(name)
+        else
+          hash[name] = send(name)
        end
      end
    end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 bf4 merged commit 5706e7d into rails-api:master Oct 7, 2015
@Rodrigora Rodrigora deleted the serializer-cache-inheritance branch October 8, 2015 14:38
@Rodrigora
Copy link
Contributor Author

Thanks @bf4!

After working on this PR, I realized that there are many _cache_* attributes in Serializer class.
Do you think it worth to move all _cache_* attributes for other class (maybe Serializer::CacheConfiguration) for clarity?

@bf4
Copy link
Member

bf4 commented Oct 8, 2015

@bf4 bf4 mentioned this pull request Dec 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inheritance of serializer does not inherits the cache!
2 participants