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.

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

Implement support for collection serializers. #61

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

drn
Copy link
Member

@drn drn commented Jul 24, 2016

The implements support for namespace and version serializer resolution in collection serializers.

@dblock if you have any knowledge of how asm handles the each_serializer option, that would be great info, but using the serializer option is the only way I've gotten it to work. I tried passing the each_serializer options around as both an adapter and a serializer option, but have run into issues with both.

serializer_class = resource_defined_class
serializer_class ||= collection_class
serializer_class ||= options[:serializer]
@serializer_class ||= (
Copy link
Member

Choose a reason for hiding this comment

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

Is this even valid Ruby? ;)

I think it should be

serializer_class ||= begin
   ...
end

Copy link
Member

Choose a reason for hiding this comment

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

Even better:

return if @serializer_class
@serialiazer_class = resource_defined_class
@serialiazer_class ||= collection_class
@serialiazer_class ...

@dblock
Copy link
Member

dblock commented Jul 25, 2016

Needs CHANGELOG too I believe.

I am afraid I don't know more about your question ;(

@drn drn force-pushed the collection-serializer-support branch from 48505eb to af3ccab Compare July 25, 2016 16:47
@drn
Copy link
Member Author

drn commented Jul 25, 2016

@dblock - Thanks for the comments! I've addressed them, updated the changelog with whatever pulls were missing, and updated the pull.

@drn drn mentioned this pull request Jul 25, 2016
Closed
end

private

attr_accessor :resource, :options

def serializer_class
return @serializer_class if defined?(@serializer_class)
serializer_class = resource_defined_class
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just use @serializer_class?

Copy link
Member Author

Choose a reason for hiding this comment

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

good call, ill switch to that

@dblock
Copy link
Member

dblock commented Jul 26, 2016

More questions, but this is very close to merging.

@drn drn force-pushed the collection-serializer-support branch from af3ccab to 6bb2057 Compare July 26, 2016 17:21
@drn
Copy link
Member Author

drn commented Jul 26, 2016

@dblock - Updated addressing your comments! Thanks again

@dblock dblock merged commit eda39c5 into ruby-grape:master Jul 26, 2016
@dblock
Copy link
Member

dblock commented Jul 26, 2016

Merged, thanks for hanging in here!

@drn
Copy link
Member Author

drn commented Jul 26, 2016

Cool, no prob. Thanks for the merge!

@drn drn deleted the collection-serializer-support branch July 26, 2016 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants