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

#10161 Fix custom migration folder path #11071

Closed
wants to merge 2 commits into from

Conversation

morcmarc
Copy link

Removed hard coded migration folder path from the create new migration method.

…older path set in the Rails application config
@@ -8,7 +8,7 @@ class MigrationGenerator < Base # :nodoc:
def create_migration_file
set_local_assigns!
validate_file_name!
migration_template @migration_template, "db/migrate/#{file_name}.rb"
migration_template @migration_template, "#{Rails.application.config.paths['db/migrate'].first.to_s}/#{file_name}.rb"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. This should indeed be fixed but we are not supposed to access the configuration direclty. Instead, we should copy its value to somewhere like ActiveRecord::Base.migration_path on intiailization and rely on it instead. We also need a test to avoid future regressions. :)

@schneems
Copy link
Member

2 months no activity @morcmarc have you had a chance to implement @josevalim's suggestions? Will you be able to continue working on this PR?

@morcmarc
Copy link
Author

Ah sorry guys, busy summer. I'll try to implement the suggested improvement and submit another pull request this week.

@senny
Copy link
Member

senny commented Nov 15, 2013

@morcmarc it's been another three months. Are you still up to work on this or should we see if someone takes over?

@rosenfeld
Copy link
Contributor

@josevalim, how about the implementation in PR #13376?

@rosenfeld
Copy link
Contributor

It's not an error, but a feature instead from my point of view. It would allow me to get rid of this file: https://github.com/rosenfeld/active_record_migrations/blob/master/lib/active_record_migrations/generators/migration.rb

@josevalim
Copy link
Contributor

The implementation in #13376 is still not ideal because it is relying on
the rake specific stuff to be set (unless it is guaranteed to always be
loaded when running generators). Another option is to store the value in
Active Record (or even in the ActiveRecord::Railtie) so it can be accessed
later.

@rosenfeld
Copy link
Contributor

I can't think of any use case where the rake specific stuff would not be set and the generator would still be run.

@rosenfeld
Copy link
Contributor

How about adding this path in ActiveRecord::Railtie in this initializer:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/railtie.rb#L113-L119

@rafaelfranca
Copy link
Member

Closing due inactivity. Thank you for the pull request.

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.

None yet

6 participants