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
Controller generator improvements #45627
Controller generator improvements #45627
Conversation
- Arguments are actions, not views - URLs are routes - Adds example for --skip-routes option
if parent | ||
parent | ||
else | ||
"ApplicationController" | ||
end |
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.
if parent | |
parent | |
else | |
"ApplicationController" | |
end | |
parent || "ApplicationController" |
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 think it will be a good idea to enable the relevant rubocop's check - https://docs.rubocop.org/rubocop/1.31/cops_style.html#styleredundantcondition
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 agree this would be better addressed by enabling the cop. Looks like there's only 3 offenses in all of Rails and I've introduced 2 of them. 😅
Would you like to open a PR to introduce this @fatkodima?
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.
Good idea @fatkodima ! 👍
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.
instead of adding this method, could a default: "ApplicationController"
be specified on the class_option
?
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.
Yes, but I think a more sweeping change could be made to change the other generators too so they look the same, more or less. I'll open another PR to do that. It is likely better to state the default in the class opt so it shows up in the help text.
…f controller. Example: `bin/rails g controller admin/users --parent=admin_controller` generates: ```ruby class Admin::UsersController < AdminController # ... end ```
0992ea1
to
aaeffb2
Compare
|
||
`bin/rails generate controller users index --skip-routes` | ||
|
||
This generates a `UsersController` with an index acion and no routes. |
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.
acion -> action
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. Do you want to open a PR?
Fixes typo introduced in rails#45627.
Summary
Update controller generator description:
Before:
After:
And add a
--parent
option to the controller generator to specify a superclass. similar to the exist ones on the job and model generators.