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

[RFC] Infer serializer from api namespace. #58

Closed
wants to merge 1 commit into from

Conversation

drn
Copy link
Member

@drn drn commented Jul 16, 2016

@nhinze - I tested this against your project and it will solve your issue. You can try it out with the following:

gem 'grape-active_model_serializers', github: 'ruby-grape/grape-active_model_serializers', branch: 'namespace-inferred-serializer'

@dblock, @jrhe, @siong1987 - This change makes it so that the serializer lookup logic attempts to use the api namespace as the namespace of the serializer for both collection and resource inputs. This is a quick pass at the logic and not ready to be merged in. There are a number of items that I think still need to happen:

  • Add specs for this
  • Lookup namespace using both API class name and resource instead of just API class name
    • This solves @nhinze's use case, but wont work for others. It should use only namespace and resolve the serializer name based on the resource class
  • Work with ASM maintainers to add the namespace option back into serializer lookup so we don't have to duplicate serializer lookup logic in grape-ASM
    • Because ASM doesn't accept that option anymore, grape-ASM doesn't have a hook for namespacing serializers without managing all of the lookup logic
  • Figure out how grape-ASM should actually handle serializer resolution
    • Through options[:version]? Downsides are nested namespaces don't work well.
    • Through api namespace?

Without this pull, grape-ASM tries to resolve the serializer namespace using options[:version] and it only works for non-collection inputs. Otherwise, it just uses ASM's serializer lookup logic. I think using the namespace of the requesting api as the namespace of the serializer is probably the best bet and have ASM add support back for the namespace option and ensure it passes it through from collection serializers to the member serializers. Thoughts?

I'll be traveling until Thursday, but will get back to this after I return

@drn drn force-pushed the namespace-inferred-serializer branch from 9ab8824 to 5654c4f Compare July 16, 2016 07:48
@drn drn changed the title [RFC] Infer serializer from namespace. [RFC] Infer serializer namespace from api namespace. Jul 16, 2016
@drn drn changed the title [RFC] Infer serializer namespace from api namespace. [RFC] Infer serializer from api namespace. Jul 16, 2016
@dblock
Copy link
Member

dblock commented Jul 16, 2016

Nice work, let us know when this is ready to merge.

rescue LoadError
# attempt to fetch serializer through namespace inference
serializer = namespace_inferred_serializer(options)
end if serializer.nil?
Copy link
Member

Choose a reason for hiding this comment

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

I would extract this into a method, especially that inside a method you can rescue without a begin since you can do that on the entire method.

@drn
Copy link
Member Author

drn commented Jul 16, 2016

Cool, I'm traveling until Thursday but I'll update this after then. I'm planning on refactor the logic here pretty significantly if that's cool

@dblock
Copy link
Member

dblock commented Jul 22, 2016

Now that #59 is merged, this needs a rebase, thanks.

@dblock
Copy link
Member

dblock commented Jul 24, 2016

This is superseded by #60 and can be closed, right?

@drn
Copy link
Member Author

drn commented Jul 24, 2016

Yup! Closing

@drn drn closed this Jul 24, 2016
@drn drn deleted the namespace-inferred-serializer branch April 26, 2017 02:28
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