Simplify attributes handling.

[beauby]

ref: #1356 (comment)

Rationale: I believe the level of abstraction introduced here adds more complexity than required.

[bf4]

On first read looks great!

BTW, that's why I set the things to api private :)

[beauby]

@bf4 yup, sorry I didn't make it in time for #1356. I'm not sure this PR deserves a changelog though.

[bf4]

Up to you

B mobile phone

On Dec 11, 2015, at 4:21 PM, Lucas Hosseini [email protected] wrote:

@bf4 yup, sorry I didn't make it in time for #1356. I'm not sure this PR deserves a changelog though.

—
Reply to this email directly or view it on GitHub.

[beauby]

Added a commit to avoid storing a lambda calling a proc. How are we feeling on this PR?

[bf4]

• so, I kind of like the polymorphism better.
• but at the end of the day, this is less code and the switch is entirely contained here, which is good
• on the other hand, now there's a data clump of _attribute_procs and _attribute_keys passed around
• and _attribute_keys sounds a lot like _attributes_keys, which seems to be a bug attractor to me
• but _attribute_value(name) is a nicer interface than attribute_mapping.call(self) and is more consistent with the reflection value

So, how about

class Attribute
  attr_reader :name, :block
  def initialize(attr, block = nil)
    @name = attr
    @block = block
  end
  def value(instance)
    if @block
      instance.instance_eval(&block)
    else
      instance.read_attribute_for_serialization(name) 
    end
  end
end

and we've removed storing the unnecessary lambdas, introduced a smaller change to the serializer interface, and isolated the attribute vs. block switch with a consistent interface to Reflection?

         def attribute(attr, options = {}, &block)
           key = options.fetch(:key, attr)
           _attribute_mappings[key] = Attribute.new(attr, block)

[bf4]

@beauby your thoughts?

[bf4]

failures in CI probably fixed by #1384

[beauby]

@bf4 How about now?

[bf4]

I feel better about this l, since it appears to be a pattern we're using all over. However, I still don't have a good sense if we'll want to keep this style, and would like to mark the object as part of the internal api.

[beauby]

Agreed. Every reference to an Attribute object is currently marked as @api private.

[bf4]

I'd prefer the autoload below the extend ActiveSupport::Concern. I believe that's how we're doing it elsewhere. Any particular reason it's here?

[bf4]

or should it be auto-loaded at all?

[beauby]

I don't feel strongly about this but I followed what was in Associations. However, I didn't put the extend ActiveSupport::Autoload, which usually goes above the autoload :things. What's the use of extend ActiveSupport::Autoload?

[bf4]

It's a different autoload from the one in ruby. I forget what it adds. Autoload is now thread safe in our supported rubies

B mobile phone

On Dec 27, 2015, at 7:19 PM, Lucas Hosseini [email protected] wrote:

In lib/active_model/serializer/attributes.rb:

     end

•    autoload :Attribute
  

  I don't feel strongly about this but I followed what was in Associations. However, I didn't put the extend ActiveSupport::Autoload, which usually goes above the autoload :things. What's the use of extend ActiveSupport::Autoload?

—
Reply to this email directly or view it on GitHub.

[bf4]

• I don't know that referring to this as a mixin is a useful changelog entry, but I'm going to merge it.. just to end the back and forth on it ;)

Simplify attributes handling via a mixin