-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add PORO serializable base class: ActiveModelSerializers::Model #1272
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# ActiveModelSerializers::Model is a convenient | ||
# serializable class to inherit from when making | ||
# serializable non-activerecord objects. | ||
module ActiveModelSerializers | ||
class Model | ||
include ActiveModel::Model | ||
include ActiveModel::Serializers::JSON | ||
|
||
attr_reader :attributes | ||
|
||
def initialize(attributes = {}) | ||
@attributes = attributes | ||
super | ||
end | ||
|
||
# Defaults to the downcased model name. | ||
def id | ||
attributes.fetch(:id) { self.class.name.downcase } | ||
end | ||
|
||
# Defaults to the downcased model name and updated_at | ||
def cache_key | ||
attributes.fetch(:cache_key) { "#{self.class.name.downcase}/#{id}-#{updated_at.strftime("%Y%m%d%H%M%S%9N")}" } | ||
end | ||
|
||
# Defaults to the time the serializer file was modified. | ||
def updated_at | ||
attributes.fetch(:updated_at) { File.mtime(__FILE__) } | ||
end | ||
|
||
def read_attribute_for_serialization(key) | ||
if key == :id || key == 'id' | ||
attributes.fetch(key) { id } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the id method is missing, I probably shouldn't be trying to call it here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But, original code was - if name == :id || name == 'id'
- id There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is just an example, is there anything wrong with just having attributes[key] ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's used by the test suite like this... I'll double check tonight B mobile phone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if id method is missing, shouldn't the it fail as a object complient resource? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NullVoxPopuli I ran some tests, and if only attributes[key] is present, it returns nil when the key is not present on the attributes hash
cc @bf4 |
||
else | ||
attributes[key] | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ def test_render_using_adapter_override | |
|
||
def test_render_skipping_adapter | ||
get :render_skipping_adapter | ||
assert_equal '{"attributes":{"name":"Name 1","description":"Description 1","comments":"Comments 1"}}', response.body | ||
assert_equal '{"name":"Name 1","description":"Description 1","comments":"Comments 1"}', response.body | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. going forward, can the expected value be assigned to an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, would be good B mobile phone
|
||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
require 'test_helper' | ||
|
||
class ActiveModelSerializers::ModelTest < Minitest::Test | ||
include ActiveModel::Serializer::Lint::Tests | ||
|
||
def setup | ||
@resource = ActiveModelSerializers::Model.new | ||
end | ||
end |
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.
if we make attributes a hash with indifferent access, we wouldn't need this
||
check.I just think it's a little weird to only have indifferent access for the id.
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 was going to explain it but now I'm not sure
B mobile phone
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.
For reasons I don't recall there might be an
id
method but not and:id
attribute but it still hits method_missing...