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

Add some failing tests around has_many assocs ..where no serializer is defined for the thing that is has_many'd #955

Closed

Conversation

JustinAiken
Copy link
Contributor

As requested in #877

..where no serializer is defined for the thing that is has_many'd
@JustinAiken
Copy link
Contributor Author

The three tests are probably redundant, but I can remove which ever ones aren't helpful.

@joaomdmoura
Copy link
Member

Great @JustinAiken, now we can keep working into this branch using the tests as reference 😄

@bf4
Copy link
Member

bf4 commented Jun 21, 2015

In looking at this, is the problem that we have a record with a serializer, and one of its associations has a an enumerable object such as an array of hash whose elements don't implement ActiveModel::Serialization? So is the problem in not having a pass-through-serializer that arrayserializer can call or in calling the array serializer in the first place? I'm been playing with this a bit, and think it might just be better to not use AMS to serialize objects that it doesn't know how to serialize... that's the renderer's job, no?

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 21, 2015
@bf4
Copy link
Member

bf4 commented Jun 21, 2015

This works inasmuch as it doesn't crash, but I'm not sure if the end behavior is what we want 56a7bd1 fwiw (that commit is orphaned, not merged anywhere)

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 22, 2015
@bf4
Copy link
Member

bf4 commented Jun 22, 2015

The commit in this PR is now included in #962. Propose closing (and thanks!) 😄

@joaomdmoura
Copy link
Member

Just waiting the merge before closing all issues related to it @bf4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants