Respect namespacing in SerializerResolver#93
Conversation
| def self.resolve(name) | ||
| serializer_name = "#{name.singularize.camelize}Serializer" | ||
| serializer_const = safe_const_get(serializer_name) | ||
| class << self |
There was a problem hiding this comment.
I took the liverty to convert to class << self, as it makes it easier to have private class method. Previously is_serializer and safe_const_get were actually public.
| def self.is_serializer(const) | ||
| const < Panko::Serializer | ||
| end | ||
| if Module.method_defined?(:module_parent_name) |
There was a problem hiding this comment.
Rails 6.0 renamed Module#parent_name into Module#module_parent_name, so to support Rails 5.2 we need to do some feature checking.
|
Hum, hold on, there's some issue with the feature testing. |
casperisfine
force-pushed
the
serializer-namespace
branch
from
3a8cd6a to
385c3a5
Compare
| # frozen_string_literal: true | ||
|
|
||
| require 'active_support/core_ext/string/inflections' | ||
| require 'active_support/core_ext/module/introspection' |
There was a problem hiding this comment.
This is the two AS features used by this file.
| spec.extensions << "ext/panko_serializer/extconf.rb" | ||
|
|
||
| spec.add_dependency "oj", "~> 3.10.0" | ||
| spec.add_dependency "activesupport" |
There was a problem hiding this comment.
Since the resolver uses a bunch of ActiveSupport core_ext, it's better to add it to the dependencies.
There was a problem hiding this comment.
It was always a question for me, whether I want a direct dependency on rails for Panko since it can serialize plain ruby objects. but to be honest with me, it's 90% of the project, so ok. 👍
There was a problem hiding this comment.
Up to you, we can also try to eliminate the dependency.
camelize for instance is very easy to get rid of, singularize however it's quite trickier.
But maybe there's a way to have this code only called in Rails context? I don't know.
yosiat
left a comment
yosiat
left a comment
There was a problem hiding this comment.
LGTM, Thanks for your contribution!
Let me know if you want me to release a version or wait.
That would be awesome, as I have Shopify/shipit-engine#1139 green, but I can't ship it using a git gem. |
|
I'll wait for the CI to pass on latest master (pushed version bump for Oj), once it's passing I'll release 0.7.5. |
|
@casperisfine Thanks for all of your contributions! released 0.7.5 https://github.com/panko-serializer/panko_serializer/releases/tag/v0.7.5 |
|
Welcome, thanks for the merges and the release! |
3 participants
Context
I'm trying to convert one of our app that uses AMS 0.9 (Shopify/shipit-engine#1139), and one small issue I ran into is that Shipit is a Rails engine, as such all constants are namespaced under
Shipit::.So none of the automatic serializer resolution worked because it would look for e.g.
::UserSerializerinstead ofShipit::UserSerializer.This PR
We simply first look for a serializer inside the namespace we're defining the relation, if not found we fallback to the top level namespace.
PS: thanks for the two very quick merges!