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

Include namespace when looking up serializer for parent class if one … #2242

Conversation

artinboghosian
Copy link

…was passed

Purpose

To consider namespace, if one was passed, when looking up serializer for parent of a subclass.

Changes

Pass namespace along when looking up serializer for superclass of resource.

Caveats

Related GitHub issues

Additional helpful information

def test_serializer_inherited_serializer_with_namespace
serializer = ActiveModel::Serializer.serializer_for(@my_profile, namespace: SerializerNamespace)
assert_equal SerializerNamespace::ProfileSerializer, serializer
end
Copy link
Member

Choose a reason for hiding this comment

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

this fails without this change?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. The reason is that when looking up the serializer for superclass, in #get_serializer_for the namespace is not passed along in the method call.

@dpogue
Copy link

dpogue commented Jul 10, 2019

This is a long-standing well-known issue, and previous pull requests to fix it were ignored 😞 Hope you have better luck getting this one merged!

@wasifhossain
Copy link
Member

wasifhossain commented Jul 10, 2019

@dpogue thanks for bringing this into attention once again 👍

can you please rebase your PR (#2172) including a supporting test, so that we can continue the discussion there? having all the existing PR references in that PR description would be nice too.

@dpogue
Copy link

dpogue commented Jul 10, 2019

@wasifhossain Done, thanks!

@wasifhossain
Copy link
Member

closing this in favor of #2172

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.

4 participants