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

Remove Adapter autoloads in favor of require #1177

Merged
merged 1 commit into from
Sep 18, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Sep 18, 2015

Adapters must be eager loaded to ensure they are defined
before they are used as namespacing.

cf6a074#diff-41f2b3509d33e1c65bb70ee0ec7a2eea

Follows #1171

@bf4 bf4 force-pushed the remove_adapter_autoloads branch 3 times, most recently from f2fcdb7 to 46104e4 Compare September 18, 2015 16:13
@@ -1,4 +1,7 @@
class ActiveModel::Serializer::Adapter::Json < ActiveModel::Serializer::Adapter
module ActiveModel
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember a commit going the other way a couple weeks ago. Ha

@NullVoxPopuli
Copy link
Contributor

👍

@bf4 bf4 force-pushed the remove_adapter_autoloads branch from 46104e4 to 0c85b61 Compare September 18, 2015 17:04
Adapters must be eager loaded to ensure they are defined
before they are used as namespacing.

rails-api@cf6a074#diff-41f2b3509d33e1c65bb70ee0ec7a2eea
@bf4 bf4 force-pushed the remove_adapter_autoloads branch from 0c85b61 to ad2ca3b Compare September 18, 2015 17:45
@jfelchner
Copy link
Contributor

@bf4 Glad to see we can keep the nested structure now that the autoloads are removed. Two wins for the price of one. :)

@NullVoxPopuli
Copy link
Contributor

This is good, and @bf4 that is one crazy detailed commit message.

thanks!

NullVoxPopuli added a commit that referenced this pull request Sep 18, 2015
Remove Adapter autoloads in favor of require
@NullVoxPopuli NullVoxPopuli merged commit 7c82258 into rails-api:master Sep 18, 2015
@bf4 bf4 mentioned this pull request Sep 18, 2015
2 tasks
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 20, 2015
Breaking change:
- Adapters now inherit Adapter::Base
- 'Adapter' is now a module, no longer a class
Why?

- using a class as a namespace that you also inherit from is complicated and circular at time i.e.
  buggy (see rails-api#1177)
- The class methods on Adapter aren't necessarily related to the instance methods, they're more
    Adapter functions
- named `Base` because it's a Rails-ism
- It helps to isolate and highlight what the Adapter interface actually is
@bf4 bf4 deleted the remove_adapter_autoloads branch September 20, 2015 19:36
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.

3 participants