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

Add migrations_paths option to model generator #33994

Merged
merged 1 commit into from Sep 27, 2018

Conversation

Projects
None yet
5 participants
@gmcgibbon
Member

gmcgibbon commented Sep 26, 2018

Summary

Followup on #33760. Adds support for custom migrations path on rails generate model. Example:

bin/rails g model Room capacity:integer --migrations-paths=db/kingston_migrate
      invoke  active_record
      create    db/kingston_migrate/20180830151055_create_rooms.rb

r? @eileencodes
/cc @rafaelfranca

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Sep 26, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Sep 26, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@gmcgibbon

This comment has been minimized.

Show comment
Hide comment
@gmcgibbon

gmcgibbon Sep 26, 2018

Member

This also fixes scaffold, I'll add a test for this.

Member

gmcgibbon commented Sep 26, 2018

This also fixes scaffold, I'll add a test for this.

@rafaelfranca rafaelfranca merged commit 8a0194f into rails:master Sep 27, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gmcgibbon gmcgibbon deleted the gmcgibbon:rails_g_model_migrations_paths branch Sep 27, 2018

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Sep 27, 2018

Member

Looks good thanks!

Member

eileencodes commented Sep 27, 2018

Looks good thanks!

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 27, 2018

Member

I'm curious why the user facing API is to pass the migration paths and not the simpler db name?

Perhaps I'm extrapolating from the simple multi db database.yml with the primary database and then the animals.

But wouldn't it be easier to pass --db=animals and let Rails infer the migration paths from database.yml?

Member

kaspth commented Sep 27, 2018

I'm curious why the user facing API is to pass the migration paths and not the simpler db name?

Perhaps I'm extrapolating from the simple multi db database.yml with the primary database and then the animals.

But wouldn't it be easier to pass --db=animals and let Rails infer the migration paths from database.yml?

@gmcgibbon

This comment has been minimized.

Show comment
Hide comment
@gmcgibbon

gmcgibbon Sep 27, 2018

Member

@kaspth I like that idea, maybe we should add support for both? The current implementation would still be useful for edge cases, I think.

Member

gmcgibbon commented Sep 27, 2018

@kaspth I like that idea, maybe we should add support for both? The current implementation would still be useful for edge cases, I think.

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Sep 27, 2018

Member

Let's change it to --database and have that lookup the database migrations paths from the configuration - assuming that the configuration is available to look it up from.

Member

eileencodes commented Sep 27, 2018

Let's change it to --database and have that lookup the database migrations paths from the configuration - assuming that the configuration is available to look it up from.

@gmcgibbon

This comment has been minimized.

Show comment
Hide comment
@gmcgibbon

gmcgibbon Sep 27, 2018

Member

Sounds good, I'll get to work on it!

Member

gmcgibbon commented Sep 27, 2018

Sounds good, I'll get to work on it!

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 15, 2018

Unify changelog entries related to `database` option of Rails generat…
…ors [ci skip]

`migrations_paths` option was added to migration generator, with
changelog entry, in rails#33760.
Also `migrations_paths` option was added to model generator, with
changelog entry, in rails#33994.
Then `migrations_paths` was renamed to `database` and aliased as `db`
in rails#34021, and was added new changelog entry.
I think we should edit existed changelog entries instead adding new
about changing the name of the option from `migrations_paths` to `database`
since Rails 6.0 hasn't been released yet, and since It might confuse
readers of the changelog file in case if they've read changelog enty about
adding `migrations_paths` option but haven't read the entry about
change the name of that option to `database`.
@eileencodes, @gmcgibbon, @rafaelfranca Does it make sense?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment