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.

Sign up for GitHub

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

Jump to bottom

Replace fail/rescue CollectionSerializer::NoSerializerError with throw/catch :no_serializer #1767

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Replace fail/rescue CollectionSerializer::NoSerializerError with throw/catch :no_serializer #1767

merged 1 commit into from
Sep 26, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Jun 1, 2016 •
edited
Loading

Closes #1693

Fixes #1075

@groyoh
Copy link
Member

groyoh commented Jun 2, 2016

What was wrong about fail/rescue here?

@bf4
Copy link
Member Author

bf4 commented Jun 2, 2016

It's not a failure...

B mobile phone

On Jun 2, 2016, at 1:23 AM, Yohan Robert notifications@github.com wrote:

What was wrong about fail/rescue here?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@NullVoxPopuli
Copy link
Contributor

I dig it. Slightly simpler.

@bf4 bf4 force-pushed the serializer_cleanup_5 branch from 6373a4f to 4f10640 Compare June 6, 2016 05:12
bf4
bf4 reviewed Jun 6, 2016
View reviewed changes
CHANGELOG.md
@@ -6,6 +6,7 @@ Breaking changes:

Features:
- [#1426](https://github.com/rails-api/active_model_serializers/pull/1426) Add ActiveModelSerializers.config.default_includes (@empact)
- [TBD](TBD) Replace raising/rescuing `CollectionSerializer::NoSerializerError`, throw/catch `:no_serializer`. (@bf4)
Copy link
Member Author

Choose a reason for hiding this comment

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

:!!!

@bf4 bf4 force-pushed the serializer_cleanup_5 branch from 4f10640 to c2d267f Compare June 9, 2016 06:44
bf4
bf4 reviewed Jun 9, 2016
View reviewed changes
lib/active_model/serializer/collection_serializer.rb
if serializer_class.nil? # rubocop:disable Style/GuardClause
fail NoSerializerError, "No serializer found for resource: #{resource.inspect}"
if serializer_class.nil?
ActiveModelSerializers.logger.debug "No serializer found for resource: #{resource.inspect}"
Copy link
Member Author

Choose a reason for hiding this comment

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

If we merge this, it fixes #1693 I believe cc @beauby

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it does, although we should also warn when we can't find a serializer for something that is not inside a collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean at serializer lookup on rendered resource but on associations? Or just always debug?

B mobile phone

On Jun 9, 2016, at 4:09 AM, Lucas Hosseini notifications@github.com wrote:

In lib/active_model/serializer/collection_serializer.rb:

@@ -73,8 +72,9 @@ def serializers_from_resources
def serializer_from_resource(resource, serializer_context_class, options)
serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) }

  •    if serializer_class.nil? # rubocop:disable Style/GuardClause

  •      fail NoSerializerError, "No serializer found for resource: #{resource.inspect}"

  •    if serializer_class.nil?

  •      ActiveModelSerializers.logger.debug "No serializer found for resource: #{resource.inspect}"
    I think it does, although we should also warn when we can't find a serializer for something that is not inside a collection.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@bf4 bf4 mentioned this pull request Jun 9, 2016
Closed
beauby
beauby reviewed Jun 9, 2016
View reviewed changes
docs/ARCHITECTURE.md
@@ -15,7 +15,7 @@ It requires an adapter to transform its attributes into a JSON document; it cann
It may be useful to think of it as a
[presenter](http://blog.steveklabnik.com/posts/2011-09-09-better-ruby-presenters).

The **`ActiveModel::ArraySerializer`** represent a collection of resources as serializers
The **`ActiveModel::CollectionSerializer`** represent a collection of resources as serializers
Copy link
Contributor

Choose a reason for hiding this comment

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

s/represent/represents/

@bf4 bf4 mentioned this pull request Aug 2, 2016
Closed
@bf4 bf4 force-pushed the serializer_cleanup_5 branch 5 times, most recently from c803432 to 5b6cd92 Compare September 1, 2016 14:13
@remear
Copy link
Member

remear commented Sep 16, 2016

This seems fine to me. What's holding this up apart from a rebase?

@NullVoxPopuli
Copy link
Contributor

@remear I don't think so. I can rebase this. @bf4 has been super busy

@NullVoxPopuli
Copy link
Contributor

So, there are 4 failing tests. they all have to do with meta, and I could find anything in our docs saying how to use meta in the way that it's being used in the failing tests, so I wanted to ask you guys (@remear and @bf4) if the usage in the failing tests is correct

@bf4
Copy link
Member Author

bf4 commented Sep 26, 2016

@NullVoxPopuli @remear issues with failing tests should be resolved/clarified by #1904

I might broke the tests or behavior here in an earlier rebase, but wasn't sure which. #1904 will make figuring that out easier :)

@bf4 bf4 force-pushed the serializer_cleanup_5 branch from af5adab to 13fa368 Compare September 26, 2016 13:53
@NullVoxPopuli
Copy link
Contributor

LGTM

@NullVoxPopuli NullVoxPopuli merged commit 6c6e45b into rails-api:master Sep 26, 2016
@NullVoxPopuli NullVoxPopuli deleted the serializer_cleanup_5 branch September 26, 2016 14:18
@bf4 bf4 mentioned this pull request Oct 31, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@beauby beauby

Labels
C: Cleanup C: Feature V: 0.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bf4 @groyoh @NullVoxPopuli @remear @beauby