Add inline syntax to override attributes/associations. #1262

beauby · 2015-10-09T18:06:56Z

This PR brings the following syntax for overriding attributes/associations and declaring virtual attributes/associations in a serializer:

class PostSerializer < ActiveModel::Serializer
  attribute :name do
    "#{object.first_name} #{object.last_name}"
  end

  has_many :comments do
    object.comments.limit(10)
  end
end

that is currently equivalent to the usual

class PostSerializer < ActiveModel::Serializer
  attribute :name
  def name
    "#{object.first_name} #{object.last_name}"
  end

  has_many :comments
  def comments
    object.comments.limit(10)
  end
end

Note that this change is not only cosmetic, as it is a prerequisite to move away from the old way (defining methods on the serializer, which caused many issues when the attribute name was that of a reserved method name (ref #1261)), and refactor the way attributes are handled in a safer way.

Update: added overriding of associations as well + factoring out of the attributes logic.

bf4 · 2015-10-09T18:14:04Z

Nice notion! But it increases complexity for a solved problem. If you want to prove the problem isn't solved, and this fixes it, use attributes named 'test' and 'select' that fail before the change and work after.

i.e

attribute :select
def select
  object.banana
end

vs.

attribute :select do
  object.banana
end

beauby · 2015-10-09T18:18:07Z

@bf4 You're right, in the current state of things, this does not solve the aforementioned problem. However, in order to solve it, we need to get rid of users defining a virtual/overridden attribute as a method on the serializer, which this PR offers a way to.
The next PR in this series will extract attributes handling into a mixin and store the attributes' values into a hash instead of defining a method on the serializer.

bf4 · 2015-10-09T22:48:12Z

I still don't see how this helps in any way. Show me! ;)

B mobile phone

On Oct 9, 2015, at 1:18 PM, Lucas Hosseini notifications@github.com wrote:

@bf4 You're right, in the current state of things, this does not solve the aforementioned problem. However, in order to solve it, we need to get rid of users defining a virtual/overridden attribute as a method on the serializer, which this PR offers a way to.
The next PR in this series will extract attributes handling into a mixin and store the attributes' values into a hash instead of defining a method on the serializer.

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

beauby · 2015-10-10T13:32:25Z

@bf4 See #1263. Once we remove the public_send in attribute_value, no more clash with existing methods.

bf4 · 2015-10-10T15:21:41Z

Well, I still want to hold on this

B mobile phone

On Oct 10, 2015, at 8:32 AM, Lucas Hosseini notifications@github.com wrote:

@bf4 See #1263. Once we remove the public_send in attribute_value, no more clash with existing methods.

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

beauby · 2015-10-10T15:22:40Z

@bf4 Sure, let's talk about it when you're back.

bf4 · 2015-10-12T00:59:58Z

test/serializers/attribute_test.rb

+      end
+
+      def test_virtual_attribute_block
+        post = PostWithVirtualAttribute.new(first_name: 'Lucas', last_name: 'Hosseini')


in this case, you might want to test post.attributes since that's the immediate case.. tests less layers

I'd like to see this too.

beauby · 2015-10-21T21:36:05Z

lib/active_model/serializer/attributes.rb

+          end
+        end
+
+        def attribute(attr, options = {}, &block)


Added the &block param.

bf4 · 2015-10-22T03:12:21Z

I'm still very 👎 on this. Sorry. If you weren't a collaborator I would close it.

beauby · 2015-10-22T03:16:47Z

@bf4 Any specific reason?

bf4 · 2015-10-22T03:30:10Z

I don't think it's good for AMS.

beauby · 2015-10-22T03:31:48Z

Would you care to give

any specific reason?

beauby · 2015-10-22T03:32:37Z

Because as you can imagine, I do think it's good for AMS, so it's kind of a Mexican standoff.

bf4 · 2015-10-22T03:52:43Z

except others are welcome to show up and approve :)

NullVoxPopuli · 2015-10-22T23:40:43Z

lib/active_model/serializer/attributes.rb

+          _attributes << key unless _attributes.include?(key)
+
+          ActiveModelSerializers.silence_warnings do
+            define_method key do


this absolutely needs to be document above the method signature with some examples.
I am a HUUUGE fan of metaprogramming, but it can be a pain in the butt if people aren't expecting it. So, having some example syntax in a method yardoc would be fabulous.

Agreed, although this code is already in AMS. I just added the if block_given?; ...; else.

Never hurts to add documentation. Code pre-existing isn't an excuse to not add docs. :-\

Sure, as I said, I agree. I was just making clear that I didn't just throw loads of metaprogramming magic without comments.

oh :-) 👍

we should be conservative in adding new interfaces because then we need to support it

if we need this feaure, why? If we don't need it, what are the trade offs of adding it? Not adding it?

how does it affect ease of understand usage? Ease of maintenaince? Complexity of either?

is it extending existing behavior or modifying it? Could it bd done another way?

how do you see this feature evolving? What are its limitations? Can it be easily customized? Eg adapter no longer need to be in thd ams namespace or even inherit the base. So, it's easy to extend (though the boundaries could be clearer and the interfaces more consitent. Am::s should probably be called a resource and implement the serializers::json interface
(Writing in phone; apologies for anything unclear)

NullVoxPopuli · 2015-10-22T23:41:05Z

ok, so --

I am a fan of, especially for the json api, how there are numerous things that can be configured via blocks. I think that's very consistent, and doesn't leave our users guessing as to which syntax to use.

I do agree that the way module inheritance has been going has been kinda sketchy, but I believe that is a bigger overall architecture discussion for another time.

@beauby, you have my 👍

@bf4, do have any specific concerns about this for AMS?

I do know some static analysis tools will complain if there is too much code at the class level, and prefer things to be done in methods (as they are more easily testable, stubbable, greppable etc)

beauby · 2015-10-25T20:02:30Z

@bf4:

we should be conservative in adding new interfaces because then we need to support it

I agree. We should also provide the best possible interfaces to our users.

if we need this feaure, why? If we don't need it, what are the trade offs of adding it? Not adding it?

This feature has two main benefits:

When overriding attributes/associations (or declaring virtual attributes/associations), it forces the user to have the redefinition next to the definition, thus encouraging code clarity.
If we want to move away from dynamically declaring methods on the serializer in order to access the object's properties (which causes clashes with existing method names, and hard to understand errors) , we need to provide a different way to override attributes/associations (which this feature does).

how does it affect ease of understand usage? Ease of maintenaince? Complexity of either?

As stated above, I believe it improves ease of usage. Complexity and ease of maintenance are hardly increased.

is it extending existing behavior or modifying it? Could it bd done another way?

Currently, it is extending existing behavior. It could be done in an other way.

how do you see this feature evolving? What are its limitations? Can it be easily customized?

I'm not sure about what you mean by evolving, limitations and customization in this setting.

Eg adapter no longer need to be in thd ams namespace or even inherit the base. So, it's easy to extend (though the boundaries could be clearer and the interfaces more consitent.

Not sure I understand. This PR does not deal with adapters in any way.

Am::s should probably be called a resource

Sounds sensible

and implement the serializers::json interface

The ActiveModel::Serializers::JSON interface is just as_json/from_json. What do you mean?

bf4 · 2015-10-27T15:55:19Z

like I said, it's pretty similar, but I don't like passing the block all over the place. I'd like a more surgical change.

bf4 · 2015-10-27T15:55:55Z

Also, what I described solves a problem. It's more than just sugar

beauby · 2015-10-28T16:53:34Z

@bf4 As I stated in the PR description, this is only part 1 of a 2 part PR:

Note that this change is not only cosmetic, as it is a prerequisite to move away from the old way (defining methods on the serializer, which caused many issues when the attribute name was that of a reserved method name (ref #1261)), and refactor the way attributes are handled in a safer way.

Plus, at the end of the day, this PR is passing a proc around, not a block.

beauby · 2015-10-28T18:52:45Z

So to sum up my vision of things here:

I'd like to introduce this inline block syntax to override attributes and associations (and declare virtual ones)
Because it promotes cleaner serializers for the users (all the logic for one attribute/association kept in one place of the file)
And also so that we can remove this nasty dynamic method generation for each attribute/association, which leads to very surprising errors (c.f. mentioned issues)
So that we're left with clean serializer classes, on which all the methods defined are AMS ones, and possibly some user-defined which could be used for
- conditionally including/excluding attributes/associations (the proposed include_if / exclude_if options, which would only take symbols (referring to user-defined methods), no lambdas)
- conditionally chosing the serializer for an association (the proposed extension of the serializer options which could take a String, a Constant, or a Symbol (referring to a user-defined method).

bf4 · 2015-10-29T05:32:19Z

passing a proc around, not a block.

I'm not sure I understand the significance of the distinction. I don't follow. It's a callable object. Proc, Lambda, Method are all objects that are all callable.

beauby · 2015-10-29T05:33:49Z

You said

I don't like passing the block all over the place

I said

this PR is passing a proc around, not a block.

bf4 · 2015-10-29T05:39:45Z

In any case, I think the change in this PR could be much smaller, solve the problem ( no need for #1263 ), and actually improve the design and clarity of the code, as I discussed in #1262 (comment)

Still needs work, IMHO. Let's focus this PR on defining/encapsulating attribute conversions at the class level. Let me know if you want to pair.

bf4 · 2015-11-10T10:04:27Z

Where are we on this? want me to submit a PR to your branch?

beauby · 2015-11-11T16:49:41Z

Will update soon.

bf4 · 2015-11-11T17:37:49Z

awesomes

beauby · 2015-11-13T16:33:07Z

@bf4 See 1311 which covers half of this PR (the associations part).

bf4 · 2015-11-13T16:36:52Z

lib/active_model/serializer/associations.rb

-        class << base
-          attr_accessor :_reflections
-        end
+        base.class_attribute :_reflections


did you intend to allow instance reader/writers? did you intend to make it inheritable?

Adapted from rails-api#1262

bf4 · 2015-11-30T23:54:53Z

@beauby I put together something somewhat minimal based on your work here #1356 (which includes your commits)

Adapted from rails-api#1262

bf4 · 2015-12-14T20:54:08Z

Closed by #1356

Adapted from rails-api#1262

bf4 added V: 0.10.x S: Needs Team Discussion labels Oct 9, 2015

beauby force-pushed the attribute-dsl branch from 6596c57 to 390e877 Compare October 9, 2015 18:22

bf4 added the S: Hold label Oct 12, 2015

bf4 reviewed Oct 12, 2015
View reviewed changes

beauby force-pushed the attribute-dsl branch from 5de2095 to d00ee4d Compare October 19, 2015 22:57

beauby changed the title ~~Add inline syntax to override attributes.~~ Add inline syntax to override attributes/associations. Oct 19, 2015

beauby force-pushed the attribute-dsl branch from d00ee4d to c7d0c59 Compare October 19, 2015 23:16

beauby reviewed Oct 21, 2015
View reviewed changes

lib/active_model/serializer/attributes.rb

end

end

def attribute(attr, options = {}, &block)

Copy link

Contributor Author

beauby Oct 21, 2015

Choose a reason for hiding this comment

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

Added the &block param.

NullVoxPopuli reviewed Oct 22, 2015
View reviewed changes

Add inline syntax to override attributes.

0c4e1f5

beauby mentioned this pull request Oct 28, 2015

Refactor attributes #1263

Closed

beauby mentioned this pull request Oct 30, 2015

Block syntax for associations. #1311

Closed

bf4 reviewed Nov 13, 2015
View reviewed changes

bf4 mentioned this pull request Nov 30, 2015

Enable use of an object key in serilaized forms #1354

Closed

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Nov 30, 2015

Allow specifying attributes with a block

773f40c

Adapted from rails-api#1262

beauby added a commit to bf4/active_model_serializers that referenced this pull request Nov 30, 2015

Add inline syntax for defining associations

225a6c9

Adapted from rails-api#1262

bf4 mentioned this pull request Nov 30, 2015

Add inline syntax for attributes and associations #1356

Merged

11 tasks

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 1, 2015

Allow specifying attributes with a block

1e1abd6

Adapted from rails-api#1262

beauby added a commit to bf4/active_model_serializers that referenced this pull request Dec 1, 2015

Add inline syntax for defining associations

5469cf4

Adapted from rails-api#1262

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 1, 2015

Allow specifying attributes with a block

b70f7e5

Adapted from rails-api#1262

beauby added a commit to bf4/active_model_serializers that referenced this pull request Dec 1, 2015

Add inline syntax for defining associations

ab6b343

Adapted from rails-api#1262

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 2, 2015

Allow specifying attributes with a block

6020450

Adapted from rails-api#1262

beauby added a commit to bf4/active_model_serializers that referenced this pull request Dec 2, 2015

Add inline syntax for defining associations

7cbef1b

Adapted from rails-api#1262

bf4 closed this Dec 14, 2015

joaomdmoura pushed a commit to joaomdmoura/active_model_serializers that referenced this pull request Jan 19, 2016

Allow specifying attributes with a block

68a34d6

Adapted from rails-api#1262

joaomdmoura pushed a commit to joaomdmoura/active_model_serializers that referenced this pull request Jan 19, 2016

Add inline syntax for defining associations

c5bce3a

Adapted from rails-api#1262

Add inline syntax to override attributes/associations. #1262

Add inline syntax to override attributes/associations. #1262

Conversation

beauby commented Oct 9, 2015

bf4 commented Oct 9, 2015

beauby commented Oct 9, 2015

bf4 commented Oct 9, 2015

beauby commented Oct 10, 2015

bf4 commented Oct 10, 2015

beauby commented Oct 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 22, 2015

beauby commented Oct 22, 2015

bf4 commented Oct 22, 2015

beauby commented Oct 22, 2015

beauby commented Oct 22, 2015

bf4 commented Oct 22, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 22, 2015

beauby commented Oct 25, 2015

bf4 commented Oct 27, 2015

bf4 commented Oct 27, 2015

beauby commented Oct 28, 2015

beauby commented Oct 28, 2015

bf4 commented Oct 29, 2015

beauby commented Oct 29, 2015

bf4 commented Oct 29, 2015

bf4 commented Nov 10, 2015

beauby commented Nov 11, 2015

bf4 commented Nov 11, 2015

beauby commented Nov 13, 2015

Choose a reason for hiding this comment

bf4 commented Nov 30, 2015

bf4 commented Dec 14, 2015