-
-
Notifications
You must be signed in to change notification settings - Fork 28
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 new #180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thank you so much @cllns!
I've left a few small requests for you. Once these are done, we can get this in :)
lib/hanami/cli/generators/gem/app.rb
Outdated
@@ -68,6 +68,15 @@ def generate_app(app, context) # rubocop:disable Metrics/AbcSize | |||
fs.write("app/assets/images/favicon.ico", file("favicon.ico")) | |||
end | |||
|
|||
if context.generate_db? | |||
fs.write("app/repo.rb", t("repo.erb", context)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a deviation from what I put in my proposal, but I wonder if this should go into app/db/repo.rb
, since then it would go alongside all the other base classes, and since it's inheriting from a class in the Hanami::DB
namespace, just like those other base classes in app/db/
.
Seeing this in full now, I reckon it should. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that makes sense to me. Should we also nest in a DB namespace like the other files in that folder? I feel like it should for consistency's sake. It could go either way since Repos are the bridge between app logic and persistence, so I don't feel strongly either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra nudge about this question, @cllns!
Here's a decision for you: let's generate this into db/repo.rb
.
With this change, we:
- Have the namespace of the class within the app echo the namespace of its superclass
- Put the app's repo base class alongside the struct base class, which feels sensible since its the job of repos to return struct instances in the first place
- Keep all the db-related support code together in one place
- Reinforce the location of
db/
as the home for all db-support code, whether this is extra ROM components (like changesets or mappers), or arbitrary code that the user wants to mix in or use alongside the other db components - Subtly reinforce that a user may choose to have other kinds of repo base classes, if they e.g. want to mix and match ROM-based repos with similarly-behaving repos of their own construction
Feels like you were happy to move in this direction, but I'm also happy to discuss further if you have any other thoughts or questions 😄
Addressed all your comments. Should we also add default DATABASE_URL's to |
Thanks @cllns, those changes look good!
I reckon we should actually add it directly to a generated Reasons:
Together, this means a single Now, as the dotenv docs point out, if you check in a To this end, I think we should:
How does this sound to you, @cllns? Once this is done I reckon we should finally be at the point of a ready-to-go database-backed app after |
p.s. I re-ran the failing tests and they turned green (due to my making the latest dry-configurable gem release), so I think this should come good after the next push. It might need a rebase to get the latest GitHub actions workflow from the main branch though (with 3.0 removed). |
Addressed all the comments and we're green again. I also added |
Right now the |
# Provide `transaction do ... end` method for database transactions | ||
include Dry::Operation::Extensions::ROM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for thinking to include this! ❤️
This is definitely good to go in now. We want these operations to support DB transactions natively.
Seeing this inspired me to create an issue to help us eliminate this line of boilerplate before 2.2 final: hanami/hanami#1437
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your persistence here, @cllns!
After moving the base repo class to db/
(per my comment here), I think this is good to merge! 🎉
Sweet. All good now, with the base repo going into |
Thanks @cllns! I'm going to merge this now, since I want to start updating our getting started guide, and being able to generate a new app will help with that :) |
Resolves #147.
A couple things.
DATABASE_URL
to.env
yet. I like the pattern of putting those into.env.development
and.env.test
. I know there was some discussion about an automatic translation from a development DATABASE_URL to a test one but not sure where we landed on that. When deploying to production, it's a feature that it won't deploy without DATABASE_URL set on the production server, so the development one isn't used accidentally.private_constant
so I can re-use them. I don't really like where they are now, any ideas of a better place to put them?Hanami::DB::Repo
and added a FIXME for when rename that toHanami::Repo