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

Attempt to support Migration and MigrationContext with Rails Version Dependency #21

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

simonireilly
Copy link
Contributor

As of Rails 5.2, the migration module has been changed.

closes #15

Instead of having paths, it now has a separate ActiveRecord::MigrationContext.

This PR is the initial work to build for rails 5.2.3

@@ -0,0 +1,27 @@
require "active_record"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module is moved into its own file from migrator.rb

@@ -0,0 +1,112 @@
require "active_record"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new migration context, as required by Rails 5.2.

This will need some work as it must act as the new module does in rails; for now it is just a placeholder

@simonireilly
Copy link
Contributor Author

@ryantownsend @paulspringett @adamdullenty

I am not an ActiveRecord Savant, so would appreciate some guidance on getting this merged.

Although it has a green build for 5.2.3 I suspect there is some missing test coverage that the new modules should have broken.

I might suggest that we use the Rails::Engine style testing for this as we have a railtie in here anyway, and we could have a dummy application that requires the new version and attempts a basic penthouse boot. WDYT?

That could be a seperate PR ahead of this which would:

  1. Bring test coverage up to >95%
  2. Add a dummy app testing path as per the normal way engines are tested

Comment on lines +6 to +19
def self.included(base)
base.class_eval do
# Verbose form of alias_method_chain which is now deprecated in ActiveSupport.
#
# This replaces the original #annouce method with #announce_with_penthouse
# but allows calling #annouce by using #announce_without_penthouse.
alias_method :announce_without_penthouse, :announce
alias_method :announce, :announce_with_penthouse
end
end

def announce_with_penthouse(message)
announce_without_penthouse("#{message} - #{current_tenant}")
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
def self.included(base)
base.class_eval do
# Verbose form of alias_method_chain which is now deprecated in ActiveSupport.
#
# This replaces the original #annouce method with #announce_with_penthouse
# but allows calling #annouce by using #announce_without_penthouse.
alias_method :announce_without_penthouse, :announce
alias_method :announce, :announce_with_penthouse
end
end
def announce_with_penthouse(message)
announce_without_penthouse("#{message} - #{current_tenant}")
end
def announce(message)
super("#{message} - #{current_tenant}")
end

end
end

ActiveRecord::Migration.send(:include, Penthouse::Migration)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
ActiveRecord::Migration.send(:include, Penthouse::Migration)
ActiveRecord::Migration.send(:prepend, Penthouse::Migration)

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.

Upgrade Penthouse to be Compatible with Active Record 5.2
1 participant