-
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
Add symbol support for Serializer.type method #1515
Conversation
d3d6656
to
ce9d119
Compare
@@ -17,7 +17,7 @@ module ClassMethods | |||
# class AdminAuthorSerializer < ActiveModel::Serializer | |||
# type 'authors' | |||
def type(type) | |||
self._type = type | |||
self._type = type ? type.to_s : nil |
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.
why check if type is nil?
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.
Because of this, we might call serializer.type(nil)
and then nil.to_s
results in ""
. I tried self._type = type if type
at first but then two specs were failling out of nowhere. It looked like something related to the cache but I could not really figure it out.
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.
type && type.to_s
B mobile phone
On Feb 15, 2016, at 1:53 AM, Yohan Robert [email protected] wrote:
In lib/active_model/serializer/type.rb:
@@ -17,7 +17,7 @@ module ClassMethods
# class AdminAuthorSerializer < ActiveModel::Serializer
# type 'authors'
def type(type)
self._type = type
Because of this, we might call serializer.type(nil) and then nil.to_s results in "". I tried self._type = type if type at first but then two specs were failling out of nowhere. It looked like something related to the cache but I could not really figure it out.self._type = type ? type.to_s : nil
—
Reply to this email directly or view it on GitHub.
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.
Ah right, it didn't come to my mind.
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.
Changed it.
a0583f1
to
9cdba4a
Compare
067d00f
to
e1bdbbf
Compare
@joaomdmoura could you check if the doc I added sounds good to you? |
The `::type` method defines the JSONAPI [type](http://jsonapi.org/format/#document-resource-object-identification) that will be rendered for this serializer. | ||
It either takes a `String` or `Symbol` as parameter. | ||
|
||
Note: This method is useful only when using the `:json_api` adapter. |
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.
though, TODO: it should replace the json_key
method and root
option..
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 there an open PR for this?
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 that I recall
👍 |
The ActiveModel::Serializer.type method now accepts symbol as paremeter: class AuthorSerializer < ActiveModel::Serializer type :profile end The test file for the type was also refactored.
LGTM 👍 |
[FEATURE] Add symbol support for Serializer.type method
The
Serializer.type
method now accepts symbol as paremeter:I don't know what you guys think about this but I usually prefer to use symbol as much as possible and thus prefer to write
type :profile
thantype 'profile'
. Furthermore this change should not have any runtime impact.I also refactored the tests to make it more DRY.