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.

Sign up for GitHub

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

Jump to bottom

Inline nested serializers #1193

Closed
wants to merge 11 commits into from
Closed

Inline nested serializers #1193

wants to merge 11 commits into from

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Sep 23, 2015

Allows people to do stuff like:

class PostSerializer < ActiveModel::Serializer
  attributes :id, :name, :body
  has_many :comments do
    attributes :id, :body
    belongs_to :author do
      attributes :name
  end
end

I'd really love for people to submit tests to try and break the implementation.

ref #1190 #1157 #1113

beauby
beauby reviewed Sep 23, 2015
View reviewed changes
lib/active_model/serializer.rb
end
return unless scope_name && !respond_to?(scope_name)
self.class.class_eval do
define_method scope_name, -> { scope }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style fix – not required for this PR.

@beauby beauby mentioned this pull request Sep 23, 2015
Closed
@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

As usual, tests are failing because ruby < 2.0.1 cannot handle namespaced constants in const_get. Will fix that soon.

@beauby beauby mentioned this pull request Sep 23, 2015
Closed
@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

Fixed \o/

This was referenced Sep 23, 2015
Closed
Closed
@NullVoxPopuli
Copy link
Contributor

👍 merging unless there are objections

@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

Wait, I have to update the changelog

@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

I'll rebase this as well, but let's wait for @bf4 and @joaomdmoura's remarks on code clarity and maybe some more tests.

@NullVoxPopuli
Copy link
Contributor

kk

@NullVoxPopuli
Copy link
Contributor

I think could be a good test, to make sure namespacing holds

PostSerializer
  -> CommentsSerializer
     -> PostSerializer # different attributes than the first

@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

@NullVoxPopuli This is done in here (the global ShareSerializer has an extra date attribute).

@beauby beauby changed the title Nested implicit serializers Inline nested serializers Sep 23, 2015
@beauby beauby added this to the 0.10 milestone Sep 23, 2015
@beauby beauby self-assigned this Sep 23, 2015
@beauby beauby force-pushed the nested-implicit-serializers branch from 89fe70c to 34ba9f6 Compare September 23, 2015 22:56
@beauby beauby mentioned this pull request Sep 24, 2015
Closed
NullVoxPopuli
NullVoxPopuli reviewed Sep 24, 2015
View reviewed changes
lib/active_model/serializer/associations.rb
@@ -39,8 +40,8 @@ def inherited(base)
# @example
# has_many :comments, serializer: CommentSummarySerializer
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add another @example here with the fancy new block syntax? :-)

NullVoxPopuli
NullVoxPopuli reviewed Sep 24, 2015
View reviewed changes
test/adapter/json/nested_serializers_test.rb

def test_nested_serializers
actual = ActiveModel::SerializableResource.new(@tweet, adapter: :json).serializable_hash
expected = { tweet: { id: 1, body: 'Tweet 1', date: 'Jan 15', author: { id: 1 }, shares: [{ id: 1, platform: 'facebook' }] } }
Copy link
Contributor

Choose a reason for hiding this comment

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

can this me multi-line formatted? with single line hashes, it can be hard to tell where some objects end (sometimes)

@beauby beauby force-pushed the nested-implicit-serializers branch from 34ba9f6 to 26a56a5 Compare September 25, 2015 14:28
@NullVoxPopuli NullVoxPopuli mentioned this pull request Sep 30, 2015
Closed
@beauby beauby force-pushed the nested-implicit-serializers branch from 26a56a5 to fae14da Compare October 2, 2015 12:59
beauby
beauby reviewed Oct 2, 2015
View reviewed changes
lib/active_model/serializer.rb
serializer_class_name = "#{klass.name}Serializer"
serializer_class = serializer_class_name.safe_constantize
# NOTE(beauby): Once we drop 1.9.3 support, we can lazify the map for better perfs.
serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the dough of this PR.

@beauby beauby mentioned this pull request Oct 2, 2015
Merged
@beauby beauby force-pushed the nested-implicit-serializers branch from 2db0240 to a1aa2c0 Compare October 2, 2015 14:28
@beauby beauby mentioned this pull request Oct 2, 2015
Closed
@beauby
Copy link
Contributor Author

beauby commented Oct 4, 2015

Closing in favor of #1225 #1226.

@beauby beauby closed this Oct 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@beauby beauby

Labels
C: Feature S: Needs Code Review V: 0.10.x
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.
2 participants
@beauby @NullVoxPopuli