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.

Sign up for GitHub

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

Jump to bottom

Model Generator Source Paths Should Allow for Customization #47181

Merged
merged 2 commits into from
May 24, 2023
Merged

Model Generator Source Paths Should Allow for Customization #47181

merged 2 commits into from
May 24, 2023

Conversation

spencerneste
Copy link
Contributor

@spencerneste spencerneste commented Jan 29, 2023 •
edited

Motivation / Background

This Pull Request has been created because #13972 was closed. It has the same content as the mentioned PR, with the addition of a test that would fail without the new code, and an instance variable initialization to silence a warning.

Detail

This Pull Request changes which source paths to consider when using the rails model generator. More detail can be found here: #13972 (comment)

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch 2 times, most recently from 762173d to 552d1b5 Compare January 30, 2023 21:26
@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch from 43a28b2 to a1ba618 Compare January 30, 2023 21:30
@spencerneste spencerneste marked this pull request as ready for review January 30, 2023 22:32
@spencerneste
Copy link
Contributor Author

@rafaelfranca Based on #13972 (comment) , mind taking a look?

This PR need tests to be merged. If anyone wants to pick it up and open a new PR feel free to do. I'm closing because it is an incomplete old PR. Thank you so much for the contribution.

@zzak
Copy link
Member

zzak commented Feb 21, 2023

Since this patch is almost the same as before (except with tests), I think it's good to add the original author as well to the commit.

You can do this by adding this to the bottom of your commit message:


Co-authored-by: Author Name <email>

Which you can find on the original patch:
https://patch-diff.githubusercontent.com/raw/rails/rails/pull/13972.patch

Thanks!

@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch from a1a7e3a to bda488a Compare February 21, 2023 15:49
@zzak
Copy link
Member

zzak commented Feb 22, 2023

@spencerneste Can you amend the commit message and remove the extra commit data from git? Also for the co-author to work there can't be any whitespace/tabs before the line. LMK if you need help! I think I can commit directly to this branch if necessary 🙏

@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch from bda488a to b473b64 Compare February 22, 2023 14:00
@spencerneste
Copy link
Contributor Author

@zzak Anything else needed here ? Thanks!

@spencerneste
Copy link
Contributor Author

@rafaelfranca @zzak Mind taking a look ?

@zzak zzak added the ready PRs ready to merge label Mar 24, 2023
rafaelfranca
rafaelfranca reviewed Mar 25, 2023
View reviewed changes
activerecord/lib/rails/generators/active_record/model/model_generator.rb Outdated
@@ -58,6 +60,16 @@ def parent_class_name
end
end

def ensure_migration_source_paths
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty much reimplementing find_in_source_paths.

Can't we just use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @rafaelfranca ! find_in_source_paths is being called by migration_template so we are using it.

The issue this PR is solving is that source_paths do not contain the correct paths for customizable create_table_migration.rb.tt templates. So the solution is to include the template_paths from the migration generator, as well a "special" path to preserve existing functionality.

I'm open to any feedback to get this functionality fixed 😃

Copy link
Member

Choose a reason for hiding this comment

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

Rails::Generators.templates_path should already be in the source_paths. We don't do any of this work in other generators and they just work.

I think the source of the problem is that we are using relative path to find the template. What we can do is append the migration template to the source path in this generator and change back the relative path.

@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch 2 times, most recently from c6111bc to 2c00779 Compare March 28, 2023 22:22
@spencerneste spencerneste force-pushed the model_generator_custom_migration_template branch from 2c00779 to 87dbe37 Compare March 28, 2023 22:38
@spencerneste
Copy link
Contributor Author

@rafaelfranca @zzak Anything else needed? Thanks!

@spencerneste
Copy link
Contributor Author

@rafaelfranca @zzak Friendly reminder for review. Thanks!

@spencerneste
Copy link
Contributor Author

@zzak Are you able to give this a review? 🙏

@spencerneste
Copy link
Contributor Author

Mind taking a quick look @rafaelfranca ? Would love to get this functionality restored, thanks !

@spencerneste @joshuaflanagan @rafaelfranca
Model Generator Source Paths Should Allow for Customization
a8fe415 
Co-authored-by: Joshua Flanagan <joshuaflanagan@gmail.com>
@rafaelfranca rafaelfranca force-pushed the model_generator_custom_migration_template branch from 7e41b3a to 8d9446b Compare May 24, 2023 22:14
@rafaelfranca rafaelfranca force-pushed the model_generator_custom_migration_template branch from 8d9446b to b070c4d Compare May 24, 2023 22:54
@rafaelfranca rafaelfranca merged commit 8f9fb1e into rails:main May 24, 2023
9 checks passed
@spencerneste spencerneste mentioned this pull request May 24, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelfranca rafaelfranca Awaiting requested review from rafaelfranca

Assignees
No one assigned
Labels
actionpack activerecord docs railties ready PRs ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@spencerneste @zzak @rafaelfranca