-
Notifications
You must be signed in to change notification settings - Fork 44
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
Run rails generate
commands automatically when Super Scaffolding
#547
Run rails generate
commands automatically when Super Scaffolding
#547
Conversation
The plan from here is to run |
cc/ @jagthedrummer @pascallaliberte @kaspth Hey everyone, I decided to tag you guys because I think this PR could go one of two ways, one of which would have a big impact on our Super Scaffolding flow. Our two options
The current state of this PRThis PR currently works with commands like the following (option 2):
My thoughtsI think 1 is closer to vanilla Rails scaffolding, but I'm afraid there will be situations in which we WON'T want to Super Scaffold all of the attributes to the new views. In this scenario, we could add a flag to say If we go with option 2, Bullet Train developers can still do things the old way, and if they're certain they want to run the migration on the spot then they can just add the What seems most intuitive to you all? I personally like 1 the most, but I think developers will get thrown off if they are too used to the original workflow. |
@gazayas, do we need to automatically run migrations before we can super scaffold things? (I don't know a lot about the internals of how super scaffolding works.) In general I think we should try to stick as closely as possible to the "normal Rails way", mainly so that things feel familiar and intuitive for devs that are already familiar with those patterns. So I'm kind of inclined to just tell people to run the migration themselves the normal way. For example, if I were scaffolding a project in vanilla rails I'd do something like:
And so, (possibly naively) I'd expect the same general pattern for super scaffolding:
Using a Conversely requiring a As far as how to handle fields that shouldn't be on the form, I think that if we don't run the migration for people that they'd be able to handle it "the normal Rails way". That is they could either:
And if we do run migrations for people then it's pretty easy to follow up with a second vanilla migration to add whatever extra fields are needed. AsideNot suggesting we tackle it in this PR, but it would be nice if we could get our super scaffolding commands to be closer to vanilla rails commands. I'd love to be able to do:
|
@jagthedrummer Thank you for the feedback! I realized there's a portion that I didn't explain, so I'll write that here and maybe that will clear things up.
|
@gazayas, thanks for the additional info. With that new info I agree with your initial assessment that the first option you presented is the best. (But if we want to go with the second option for some reason I think the flag should be named |
Would it be clearer if we added this as separate commands like |
@kaspth I personally like just Until we decide, I've added the
|
This PR is ready for review, I'm still working on updating the documentation though and I'll have to think through delegated types because I don't think we have a test in place for this. |
@gazayas ok, that sounds good to me! |
Looking at the documentation for delegated types, you can see that we generate a polymorphic model:
I currently don't have a solution in mind that would automatically run this Rails command, but I've at least added the |
I like this a lot. Classy. |
I've been a little thin on time this week and haven't been able to do a thorough review yet, but I'm really looking forward to getting this merged. It'll be a very nice quality of life improvement. 🎉 |
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.
I just pulled this down and played around with it and I love it! It feels much more like it's an enhancement to the Rails scaffolding procedure that I already know vs being a whole new procedure that has different types of steps than I'm used to.
Since this will change scaffolding behavior on people, but will not break existing code, I'm thinking we should do a minor
version bump when we release this one. What do y'all think @gazayas, @kaspth, @pascallaliberte?
email_field: "string", | ||
emoji_field: "string", | ||
file_field: "attachment", | ||
image: "attachment", |
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.
Should this be active_storage_image
? Or should we remove cloudinary_image
above and then have image
be a special case where we don't do a direct lookup, but instead check whether Cloudinary is active?
@@ -53,6 +113,19 @@ def check_required_options_for_attributes(scaffolding_type, attributes, child, p | |||
{} | |||
end | |||
|
|||
data_type = if type == "image" && cloudinary_enabled? | |||
"string" |
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.
Oh, here's that special case I mentioned above. Maybe we remove cloudinary_image
from that list? (Or do we want to leave it for backwards compatibility?)
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.
Maybe we remove cloudinary_image from that list? (Or do we want to leave it for backwards compatibility?)
Right, I was planning on leaving it here for backwards compatibility. However, if we were to delete it, it technically wouldn't be breaking anything so I don't feel too strongly either way right now about leaving it or deleting it.
@jagthedrummer I think doing a minor version bump makes sense! Although I will say I'm not too strong when it comes to semantic versioning. I'm curious if there's a set of guidelines we should refer to when updating? |
@gazayas, there is a guide, but I think our situation is complicated a bit by the fact that the starter repo and the |
@jagthedrummer nice, I'm onboard with the minor bump! @gazayas great work on this ❤️ |
The PR for testing super scaffolding of Addresses (bullet-train-co/bullet_train#986) landed right before the PR for automatically generating a migration during super scaffolding (#547). As a result a bug / test-failure crept in. The `Address` model `belongs_to :addressable`, which means that the relation is stored on the `Address` side, and not on the owning model. So that means that when we super scaffold an address field it turns out that we don't need to represent that in the migration for the model that will own the address.
The PR for testing super scaffolding of Addresses (bullet-train-co/bullet_train#986) landed right before the PR for automatically generating a migration during super scaffolding (#547). As a result a bug / test-failure crept in. The `Address` model `belongs_to :addressable`, which means that the relation is stored on the `Address` side, and not on the owning model. So that means that when we super scaffold an address field it turns out that we don't need to represent that in the migration for the model that will own the address.
@kaspth Thank you! Excited to use this moving forward 🚀 |
Closes bullet-train-co/bullet_train#988.