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

Use return value of link block if any #2

Merged
merged 1 commit into from
Dec 31, 2015
Merged

Use return value of link block if any #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions lib/active_model/serializer/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,7 @@ def add_included_resources_for(serializer, include_tree, primary_data, included)

def links_for(serializer)
serializer.links.each_with_object({}) do |(name, value), hash|
hash[name] =
if value.respond_to?(:call)
link = Link.new(serializer)
link.instance_eval(&value)

link.to_hash
else
value
end
hash[name] = Link.new(serializer).value(value)
Copy link
Owner

Choose a reason for hiding this comment

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

👍 to moving this logic inside the Link class, which is in line with what we've done with Attribute/Reflection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😄

end
end

Expand Down
27 changes: 19 additions & 8 deletions lib/active_model/serializer/adapter/json_api/link.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,40 @@ def initialize(serializer)

def href(value)
self._href = value
nil
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for an explicit nil return value?

end

def meta(value)
self._meta = value
nil
end

def string(value)
self._string = value
def value(value)
if value.respond_to?(:call)
string instance_eval(&value)
_string || to_hash
Copy link
Owner

Choose a reason for hiding this comment

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

I do not understand those 2 lines though. This will always return _string, unless it is falsy, which happens only if – wait, I got it, that's why meta and href return nil.
How about the following:

def initialize(serializer, link_value)
  @object = serializer.object
  @scope = serializer.scope
  if link_value.respond_to?(:call)
    instance_eval(&link_value)
  else
    string link_value
  end
end

def value
  _string || to_hash
end

Copy link
Owner

Choose a reason for hiding this comment

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

Oh actually, your idea of using the return value of the block makes for quite a cool DSL. My suggestion of moving the actual computing of the value inside the initializer still stands, but other than this I think you're right, this is the way to go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could go either way. Like I wrote in the description

This commit leaves unresolved the behavior if there is both a return value to the block (as _string) and meta and/or href are set. To be decided in the receiving PR if merged.

This is more of an interface proposal than implementation. I considered the initializer, but thought this was more similar to our other implementations, e.g. Reflection. but I don't feel strongly about it.

I'd prefer you to merge it (if you think it's an improvement), and then add a commit or two to handle the unresolved issue I just quoted, and where the logic should go internal to the 'Link'

else
value
end
end

def to_hash
return _string unless _string.nil?
protected

attr_accessor :_href, :_meta, :_string
attr_reader :object, :scope

private

def to_hash
hash = { href: _href }
hash.merge!(meta: _meta) if _meta

hash
end

protected

attr_accessor :_href, :_meta, :_string
attr_reader :object, :scope
def string(value)
self._string = value
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/adapter/json_api/links_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class LinkAuthorSerializer < ActiveModel::Serializer
link :other, '//example.com/resource'

link :yet_another do
string "//example.com/resource/#{object.id}"
"//example.com/resource/#{object.id}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the serializer API change

end
end

Expand Down