-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Move handling of root option from serializer to adapter.
#1234
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
Conversation
f8c68f8 to
6f0e992
Compare
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.
use the serializable helper to for less words, if you want
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.
Is the helper globally available? I think it should be. If so, I'll update these tests.
|
@beauby Would you clarify what |
|
@bf4: No, they were only referring to the root of the document, that's why I believe it's an adapter concern. For the "roots" of resources, we actually use the association's key. |
6f0e992 to
ec55c81
Compare
|
Updated. |
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.
this raises a huge red flag for me. Why is this changing?
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.
I believe it should not have been there in the first place. Modifying explicitly supplied options is such an antipattern. I could move this change to an other PR though.
|
is this still applicable? |
|
Done, iirc B mobile phone
|
|
Can this be closed then? |
|
Seems not: https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializable_resource.rb#L5 |
|
@beauby I would love if you'd come back to this one. Shouldn't be to hard to finish. |
|
Will take care of it over the weekend! |
|
Closing in favor of #1782. |
This option is clearly an adapter concern and it is only used in the Json adapter.
rootare not subject to inflection anymore.Most tests are failing because they are directly testing the root value on the serializers. Will fix.