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

Generate db files during hanami generate slice #197

Merged
merged 19 commits into from
Jul 12, 2024

Conversation

cllns
Copy link
Member

@cllns cllns commented Jul 10, 2024

Closes #148. This still needs some more but I'm not sure exactly what, since I'm still a little unclear on how it'll all fit together.

When I use this hanami new slice --app-db I get an error (after generation, when the app tries to boot) because config.db. isn't defined within Hanami::Slice. Do we need to add that in hanami-db?

Can you elaborate what else is needed for this PR @timriley?

@cllns cllns requested a review from timriley July 10, 2024 23:57
@timriley
Copy link
Member

timriley commented Jul 11, 2024

Hey @cllns! Thanks again for working on this 😄

I just checked this out locally and tried passing --app-db, and everything seemed to work fine for me; no errors.

Either way, I took some time to consider the --app-db and --slice-db options we had on this command, and I think we actually don't want either of them, at least not for now.

I think that having these options would lead to confusion around their purpose. For example by default, one might consider the "app db" to be the actual service that DATABASE_URL points to, and in that case, that person might think --app-db needs to be passed when generating all slices, even when this is actually not necessary. And what would happen in this situation is that this user would potentially get confused as to why they can't define their own relations in each slice.

In this case, I considered renaming this option to either --import-app-db or --use-app-relations or some variation of this, but again, none of these felt particularly satisfying as names. So until we come up with something straightforward and not confusing, I think we're better served by removing the options.

--slice-db felt confusing in similar ways. It could be interpreted as a slice having an independent db layer, even if it still points to the shared DATABASE_URL. Again, this is not the case, and this could lead to the user seeing strange errors about multiple conflicting config/db/ directories existing when slices don't actually have distinct database URLs.

In addition, by removing these options, it means we can stop generating empty config/slice.rb files in every single new slice, which I think is a benefit. These should only be needed if a user needs to do some advanced config, and at that point they can be guided by docs and create the file themselves. Removing these empty files helps keep the overall Hanami app and slice structures feel as streamlined as possible in their beginning state.

Now, how might users opt into these various DB layer arrangements without support from the generator?

Instead of --app-db, they can put config.db.import_from_parent = true straight into config/app.rb, and all slices will pick up this behaviour automatically. Then all they need to do is rm -rf slices/my_slice/relations after running the generator — that's all. And given import_from_parent defaults to false, we can this clear in a dedicated documentation page page on the topic.

And instead of --slice-db, all the user needs to do is set a MY_SLICE__DATABASE_URL, and the matching MySlice will automatically pick up that it should use a separate database. And if they run hanami generate migration --slice=my_slice, then we should automatically create the slices/my_slice/config/db/ directory required to hold the migration.'

So as you can (hopefully!) see, it's not particularly onerous to opt into either of these alternative arrangements, even with the default behaviour of our generator. And since slices won't be created all too often compared to the lower-level files in the app, I think we can tolerate this generator being a little less flexible for starters, with the benefit of it being less confusing and pushing users more towards our preferred default in the meantime.

I made a push to this PR pulling out the options, just to make it clear in the code what I've attempted to explain in so many words just above 😆 I hope that's OK!

Please let me know what you think!

Our Zeitwerk autoloading will take care of making these constants available for us.
Copy link
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

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

If you're happy with the commits I pushed here, I'm happy to see this go in! Everything else looks great 😄 Thank you!

@cllns cllns marked this pull request as ready for review July 11, 2024 15:56
@cllns
Copy link
Member Author

cllns commented Jul 11, 2024

Perfect. Definitely agree with streamlining this and making it simpler 🫡

Copy link
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

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

Thanks @cllns, let's get this in!

@cllns cllns merged commit 1409ad9 into main Jul 12, 2024
6 checks passed
@cllns cllns deleted the generate-db-files-during-hanami-generate-slice branch July 12, 2024 00:55
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.

Generate db files in generate slice
2 participants