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

generator: sync erb template #357

Merged
merged 1 commit into from
May 1, 2024

Conversation

kinsomicrote
Copy link
Contributor

This updates the ERB template to be in sync with the template in rails/rails

Fixes #341.

@kinsomicrote
Copy link
Contributor Author

kinsomicrote commented Apr 29, 2024

Hello @flavorjones 👋

Any pointers on why the CI is failing? I ran the test suite locally, and they all passed.

@javierjulio
Copy link

@kinsomicrote based on the errors occurring for the Rails 6.1 build, it looks like model_resource_name and index_helper have a different method signature at that version. So using model_resource_name as an example, the v7.1 docs differ from the v6.1 docs.

@kinsomicrote
Copy link
Contributor Author

Thank you, @javierjulio

Would it be better to leave it as it was? For example, using index_helper %>_path instead of index_helper(type: :path) %>?

@javierjulio
Copy link

@kinsomicrote you're welcome. That could work. In general though for both methods I don't know what the maintainers here would prefer. For example, they could suggest a Rails version conditional to account for the difference instead. I just wanted to help identify the reason for the failure.

Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

@kinsomicrote Thank you for submitting this! I'd prefer to use a calling convention that's compatible with Rails 6.1 (rather than introduce version checks).

@kinsomicrote
Copy link
Contributor Author

@flavorjones tests are passing now. Let me know if it's good to go.

@flavorjones
Copy link
Member

Quick note for posterity: the model_resource_name change is from rails/rails#43611 which first appeared in Rails 7.0 and fixed namespaced models.

Namespaced models (e.g. Admin::Monster) still don't work with this PR but that's probably OK as long as we have to support Rails 6.1

This updates the erb template to be in sync with the template in rails/rails
@flavorjones
Copy link
Member

I've rebased and squashed

@flavorjones
Copy link
Member

flavorjones commented May 1, 2024

@kinsomicrote Thanks so much for doing this work!!!

I'm going to open a second PR with your changes using the Rails 7.0 API so we can have a separate conversation about dropping Rails 6.1 feature support.

Edit: that PR is #359.

@flavorjones flavorjones merged commit b6cb812 into rails:main May 1, 2024
25 checks passed
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.

Tailwind scaffold template is out of sync with rails/rails ERB scaffold template
3 participants