Associations refactoring

[bolshakov]

• Move all associations related code from Serializer class to Associations module
• Introduce Reflection class hierarchy
• Introduce Association class
• Rid off Serializer#each_association
• Introduce Serializer#associations enumerator

This refactoring brakes internal API, so it affect custom adapters

Associations introspection

Before refactoring:

  serializer.each_association do |key, serializer, options|
    # ...
  end

After refactoring:

  serializer.associations.each do |association|
    # ...
  end

Reflections introspection

Instead of Serializer._associations we introduced Serializer._reflections, which is an array of
ActiveModel::Serializer::Reflection

[joaomdmoura]

👍

[joaomdmoura]

👍

[joaomdmoura]

Awesome @bolshakov, keep up the good work.
Haven't reviewed all of it yet, but don't forget to rebase and keep it in sync it with master 😄

[bolshakov]

I have couple of questions:

Association definition syntax.

It allows to write multiple associations at once:

   has_many :comments, :posts, serializer: CoolSerializer

What does it mean? Can we make backward incompatible change and disallow to specify multiple associations at once.

   has_many :comments
   has_many :posts, serializer: PostSerializer

Underscope prefixes

I can't realize why they are used in all the code. What reason to write _reflections instead of reflections.
If it happens occasionally can we rename them all?

[bolshakov]

@joaomdmoura could you review?

[joaomdmoura]

@bolshakov, answering your questions:

1. Yes, we can. It already is kind of backward incompatible. (at least not that easy to update)
2. It mostly started on the Serializer class, because we needed a variable to store the _attributes for example (because it was used in different places), and a method that would be used to define the attributes. Then it became kind a convention.

ex:

module ActiveModel
  class Serializer

    class << self
      attr_accessor :_attributes
    end

    def self.attributes(*attrs)
      attrs = attrs.first if attrs.first.class == Array
      @_attributes.concat attrs
      @_attributes.uniq!
    end

    def another_method_using_attributes
      p @_attributes
    end

  end
end

[bolshakov]

@joaomdmoura thanks for explanation!

1. Ok, i'd rather make another pull request for that changes.
2. I understand the motivation when dealing with attributes since attributes has no additional options, But when it comes to relations they may have additional options and that options have no any sense for me

For me that behavior looks error prone:

 has_many :comments, :posts, serializer: CoolSerializer

From the other hand if I think it would be convenient to maintain same interface as ActiveRecord.

[joaomdmoura]

LGTM.
Just not sure about Reflection as the name. Other options?

[bolshakov]

Another options may be Relation.
We've borrowed Reflection term from ActiveRecord.
We have two parallel hierarchies: associations definitions on class level and associations itself on instance level. It's better to have different names for them to minimize confusion.

[bolshakov]

@joaomdmoura thought?

[joaomdmoura]

Sorry for taking so long @bolshakov, I was on a business trip, but I'm willing to merge it, can you rebase it with master?

[bolshakov]

Done, Thank you!

[joaomdmoura]

Merged. Thank you! 😄

[bolshakov]

Woo hoo!