-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Empty collection root key from explicit serializer option #1618
Conversation
@@ -182,7 +182,7 @@ def as_json(adapter_opts = nil) | |||
|
|||
# Used by adapter as resource root. | |||
def json_key | |||
root || object.class.model_name.to_s.underscore | |||
root || _type || object.class.model_name.to_s.underscore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required in this PR, but makes sense
@@ -182,7 +182,7 @@ def as_json(adapter_opts = nil) | |||
|
|||
# Used by adapter as resource root. | |||
def json_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it now, shouldn't it be the responsibility of the adapter to determine the json key/root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does, but it uses information from the serializer....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see, we can set the root
as adapter option and call serializer._type
and serializer.object.class.model_name.to_s.underscore
from the adapter. Same goes for collection_serializer. What do you think?
Great job! The PR looks good. I'll be ok to merge it once the rubocop is fixed. |
…ed to collection serializer and each_searializer is specified.
e5c4ac1
to
a74d174
Compare
Running |
@@ -3,6 +3,10 @@ | |||
module ActiveModel | |||
class Serializer | |||
class CollectionSerializerTest < ActiveSupport::TestCase | |||
MessagesSerializer = Class.new(ActiveModel::Serializer) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should align on Class.new(ActiveModel::Serializer)
vs class Name; end
.
Merging when the build is done 😉 |
Fixes #1617
Closes #1537
json_key
uses_type
when root not givenbefore falling back on object class
Array instances.