Fixes the paths for generated views #221
Conversation
| expected_contents = contents_for_application_template("show") | ||
|
|
||
| run_generator [] | ||
| contents = File.read(file("app/views/admin/application/show.html.erb")) | ||
| contents = File.read(file("app/views/administrate/application/show.html.erb")) |
|
How would you like me to handle the long lines? |
|
Based on conversation on #202, I think we can switch back to @5minpause do you want to update this to revert to |
|
I gladly update this pull request. It's evening here, so you'll get this tomorrow.
|
|
@Graysonwright I updated the commit and reverted back to |
| @@ -21,7 +21,8 @@ def copy_resource_template(template_name) | |||
| end | |||
|
|
|||
| def resource_path | |||
| args.first.try(:underscore).try(:pluralize) || "application" | |||
| return "application" if args.none? || args.first == "application" | |||
| args.first.try(:underscore).try(:pluralize) | |||
There was a problem hiding this comment.
Our style guide says to avoid conditional modifiers (lines that end with conditionals). Can we re-phrase this as:
if args.none? || args.first == "application"
"application"
else
args.first.underscore.pluralize
endAlso, I think we can ditch the try() methods, because we now know that args.first is defined at this point.
|
Left a comment - other than that, looks good! |
|
Oh I wasn't aware of this rule in your style guide. Thanks for pointing this out. Let me update the commit. |
Problem: The documentation correctly states that view generated via `rails g administrate:views` should live under `app/views/admin/application/`. The generatator puts them under `app/views/admin/applications/` though. References #202. Solution: The method that creates the path needs to be adapted.
|
@Graysonwright I updated the pull request according to the style guide. Thanks for making me aware of it. |
|
Sorry for the hold-up here. I want to pull this branch down locally and see if I can come up with tests for it. Implementation looks good, though. |
|
I hate to draw out the discussion on this more, but after looking at it closer I'm not sure this change is as simple as it first appeared. Here's my thinking: Breaking down the problemThe problem is that the generator is placing the files in This happens when the user types: However, the documentation states that if you want to generate views that apply to all resources, you should run: ...without the This is a subtle distinction, but it changes this issue from a bug report to a feature request. As long as users follow the docs, the generators will work fine for them. So instead of fixing broken behavior, this PR would essentially be auto-correcting people's mis-use of the generator API. The new questionOur new question is: if users mistakenly type There's an obvious reason to do so - we've already had at least one user confused by this, and it has the potential to help people avoid a frustrating-to-track-down error. On the other hand, imagine that someone's writing a Rails app that processes job applications, and they have a model named So, thoughts on the tradeoff? |
|
I'm closing due to inactivity. If we get more requests for this, I'll gladly re-open. |
Fixes #202.
Problem:
The documentation correctly states that view generated via
rails g administrate:viewsshould live underapp/views/administrate/application/. The generatator puts them underapp/views/admin/applications/though.Solution:
The methods that create the paths need to be adapted.
Nota bene:
While running the tests, the first arg for the generator is empty. When
the generator is run inside a project, the first arg is
application.This leads to the view paths having
applicationsinstead ofapplication. This commit fixes this as well.