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

Add --repo option to schema generator #4882

Conversation

d3mcfadden
Copy link

I added a --repo option to the schema generator so apps that have multiple repos can have the ability to select what repo the schema/migration should use.

I noticed ecto.gen.migration supports this flag and assumed it would work in phoenix as well. It did not, so here it is :-)

This works on the schema generator and other "parent" generators that call it (eg: phx.gen.context)

NOTE to reviewer

Ideally, I wanted to support a custom priv config that may be set on the repo to determine the migration_path, however I noticed that will require me ensuring the repo module is compiled first. I wanted to get some advice on that before doing anything.

Add support for apps with multiple repos to specify which repo the schema
migration should be generated for.
@josevalim
Copy link
Member

I would rather support setting a custom migration directory and that's it. Changing the directory itself should be done manually IMO because you can end-up in scenarios where a context has multiple repos aliases as: Repo and that can be confusing and trying to address such corner use case through generators are not going to scale IMO. :)

@d3mcfadden
Copy link
Author

d3mcfadden commented Jul 21, 2022

@josevalim thanks for the feedback! That makes sense to me also. I'm working on an app that has 2 repos and manually have to move stuff around after using the generators. It is not that big of a deal, but I figured it may be useful to us and others.

I can update this to support --migration-path instead if you think that change would make it in. Otherwise, we can just close this out.

Thanks!

edit: would also throw out just removing the as: Repo stuff and letting the end author be responsible for doing that in the contexts.

@josevalim
Copy link
Member

The migration path bit is welcome if you think it is helpful. Your call!

@josevalim
Copy link
Member

Closing this./ If you want to send a PR for the custom migration path, it would be welcome!

@josevalim josevalim closed this Aug 10, 2022
@MikaAK
Copy link
Contributor

MikaAK commented Aug 15, 2022

I realised this was around after i created #4905 which I needed for a generator project I'm working on, I've implemented both though if you want me to strip one of the options lemme know

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