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

Move install hooks to an install method. #26

Merged
merged 2 commits into from
Aug 16, 2016
Merged

Move install hooks to an install method. #26

merged 2 commits into from
Aug 16, 2016

Conversation

lucasmazza
Copy link
Contributor

I though about moving the railtie to it's own file with a defined? check for Rails::Railtie, so gems that use this for testing don't have to depend on railties to load this, so if it makes sense I can push a commit for that on this branch.

@rails-bot
Copy link

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

lucasmazza referenced this pull request Aug 15, 2016
While changing the code to be in an `on_load` hook fixed some of the issues
with load hook order, it didn't catch all of them. Rails runs its hooks in
initializers, meaning they'll be defined after our own hooks which are not in
an initializer.

Fixes #24
@kaspth
Copy link
Contributor

kaspth commented Aug 15, 2016

Yeah, let's move it to a separate file like Active Resource does: https://github.com/rails/activeresource/blob/f8abaf13174e94d179227f352c9dd6fb8b03e0da/lib/active_resource.rb#L44 (though I'd check defined?(::Rails::Railtie)).

@kaspth kaspth merged commit 9e89329 into rails:master Aug 16, 2016
@kaspth
Copy link
Contributor

kaspth commented Aug 16, 2016

Nice! Thanks, @lucasmazza :)

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