Conversation
d486dd2 to
14beaf4
Compare
There was a problem hiding this comment.
Pull request overview
Reworks the gem into a proper Rails plugin by renaming it to rolemodel-rails, introducing a Rails Engine for generator registration, and removing generator eager-loading during host app boot.
Changes:
- Renames the gem from
rolemodel_railstorolemodel-railsand collapses the public namespace toRolemodel. - Adds a
Rolemodel::Engineto register generator support without eager-loading all generators at boot. - Updates specs, example apps, and tooling scripts to use the new gem name and loading behavior.
Reviewed changes
Copilot reviewed 16 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/helpers/example_app.rb | Updates Gemfile cleanup to remove rolemodel-rails entries in the test app. |
| spec/spec_helper.rb | Updates spec bootstrapping to load Rails + the gem and eagerly require generator classes for tests. |
| spec/rolemodel_rails_spec.rb | Updates version constant expectations to the new Rolemodel namespace. |
| spec/generators/rolemodel/github_generator_spec.rb | Updates expected template text to reference the new version file path. |
| rolemodel-rails.gemspec | Renames the gem, updates metadata, and points version loading at Rolemodel::VERSION. |
| lib/rolemodel_rails.rb | Removes the old entrypoint that eagerly loaded generators. |
| lib/rolemodel/version.rb | Moves version constant under Rolemodel module. |
| lib/rolemodel/engine.rb | Adds Rails Engine for generator registration. |
| lib/rolemodel-rails.rb | Adds new gem entrypoint that defines version constants and requires version/engine. |
| lib/generators/rolemodel/base_generator.rb | Introduces a standalone BaseGenerator (previously defined in lib/generators/rolemodel.rb). |
| lib/generators/rolemodel.rb | Removes the old generator aggregator that loaded all generators eagerly. |
| example_rails8/Gemfile / Gemfile.lock | Updates example app to depend on rolemodel-rails. |
| example_rails7/Gemfile / Gemfile.lock | Updates example app to depend on rolemodel-rails. |
| bin/recreate_legacy_example | Updates bundle add invocation to rolemodel-rails. |
| bin/recreate_current_example | Updates bundle add invocation to rolemodel-rails. |
| bin/console | Updates require to rolemodel-rails. |
| bin/bump_version | Updates paths and gemspec name used by the version bumper. |
| README.md | Updates documentation to reference rolemodel-rails. |
| Gemfile / Gemfile.lock | Updates development dependency name to rolemodel-rails. |
| .gitignore | Adds .DS_Store ignore. |
| .github/workflows/ci.yml | Updates runner and GitHub Action versions used in CI. |
Comments suppressed due to low confidence (1)
rolemodel-rails.gemspec:5
rolemodel-rails.gemspecno longer has# frozen_string_literal: trueat the top. This repo consistently uses frozen string literals in Ruby files, and removing it may introduce RuboCop/style inconsistencies.
Suggestion: add the frozen-string-literal magic comment back at the top of the gemspec (before any require).
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@OutlawAndy I've opened a new pull request, #182, to work on those changes. Once the pull request is ready, I'll request review from you. |
cce8455 to
73df8e0
Compare
# Conflicts: # Gemfile.lock # example_rails7/Gemfile.lock # example_rails8/Gemfile.lock # lib/rolemodel/version.rb
…182) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: OutlawAndy <1504753+OutlawAndy@users.noreply.github.com>
# Conflicts: # Gemfile.lock # example_rails7/Gemfile.lock # example_rails8/Gemfile.lock # lib/rolemodel/version.rb
ffa615c to
f1751c8
Compare
3be3980 to
26004ee
Compare
timirwin
left a comment
There was a problem hiding this comment.
Looks wonderful. Just a few questions to ponder. You may not need any changes if they all check out.
| run_specs: | ||
| name: Rspec | ||
| runs-on: ubuntu-latest | ||
| runs-on: blacksmith-4vcpu-ubuntu-2404 |
There was a problem hiding this comment.
@OutlawAndy Since this is public repo and running on Github, do we need to (or want to) switch to Blacksmith?
There was a problem hiding this comment.
good question. I don't know. blacksmith is significantly faster, but might be safest to switch back. 👍
| skip_brakeman: true, | ||
| skip_bundler_audit: true, | ||
| skip_ci: true, | ||
| skip_git: true, | ||
| skip_jbuilder: true, | ||
| skip_kamal: true, | ||
| skip_rubocop: true, | ||
| skip_solid: true, | ||
| skip_test: true, | ||
| skip_thruster: true, |
There was a problem hiding this comment.
Thank you for alphabetizing this list. 👍
| set -euo pipefail | ||
| IFS=$'\n\t' | ||
| set -vx | ||
| RAILS_VERSION = ARGV[0] || '8.1.2' |
There was a problem hiding this comment.
Do we need a usage comment at the top of the file?
| end | ||
|
|
||
| gem "rolemodel_rails", "> 0.20", group: :development, path: ".." | ||
| gem "rolemodel-rails", "> 0.20", group: :development, path: ".." |
There was a problem hiding this comment.
Would we want to update the version specifier too?
| gem "rolemodel-rails", "> 0.20", group: :development, path: ".." | |
| gem "rolemodel-rails", ">= 1.0", group: :development, path: ".." |
| # Replace the default in-process and non-durable queuing backend for Active Job. | ||
| config.active_job.queue_adapter = :solid_queue | ||
| config.solid_queue.connects_to = { database: { writing: :queue } } | ||
| # config.active_job.queue_adapter = :resque |
| adapter: redis | ||
| url: <%= ENV.fetch("REDIS_URL") { "redis://localhost:6379/1" } %> | ||
| channel_prefix: example_rails8_production |
There was a problem hiding this comment.
Is this the new default for Rails 8? I believe we were leaving most of the example_rails8 files as close to rails new baseline as we can so recreating would have the least amount of changes necessary. Then again, maybe this is the right call moving forward?
There was a problem hiding this comment.
This is the default for apps generated with --no-solid. That flag is also responsible for the change you highlighted in config/environments/production.rb.
I think it might be worth while to discuss what our default should be for new apps regarding the solid components. We've discussed sticking with GoodJob over solid-queue, but not what to do about cache & cable. Using either SolidCache or SolidCable requires multiple databases on Heroku & a few minor changes to development rake tasks (namely db commands. see screenshot)
| CI.run do | ||
| step "Setup", "bin/setup --skip-server" | ||
|
|
||
| step "Style: Ruby", "bin/rubocop" |
There was a problem hiding this comment.
Would we want to have rubocop verify the output of our generator's output?
There was a problem hiding this comment.
I think one of our generators sets up running the rubocop formatter against generated files.. @wesrich can you confirm?
| { | ||
| "name": "app", | ||
| "private": true, | ||
| "packageManager": "yarn@1.22.22+sha1.ac34549e6aa8e7ead463a7407e1c7390f61a6610", |
There was a problem hiding this comment.
We don't need this even though we're keeping yarn.lock? Presumably it must work or you wouldn't have committed it?
There was a problem hiding this comment.
I think this went away because we migrated to yarn4+ a while back and the example app just hadn't been regenerated until now, but I'll try and confirm that.
| gem 'stimulus-rails' | ||
| gem 'turbo-rails' | ||
|
|
||
| # Start debugger with binding.b [https://github.com/ruby/debug] |
There was a problem hiding this comment.
Should we use the alias or keep it long form (binding.break)? Fine either way if binding.b is common knowledge.
Why?
Rolemodel&RolemodelRails) which is a general anti-pattern for RubyGems.-rather than a_. e.g.rspec-rails,slim-rails,rubocop-rails, etc. This ties back into#2becauserolemodel-railstranslates toRolemodel::Rails, whilerolemodel_railstranslates toRolemodelRails.What Changed
What changed in this PR?
rolemodel-railsrecreate_current_examplescript to use the AppGenerator withdummy_appset to true. (this is how the Rails PluginGenerator does it)boot.rbfile in the example app which points to the plugin, rather then adding the plugin to it's gemfile (also how the Rails PluginGenerator does it)Future Considerations
As a "Rails Plugin" with a Railtie/Engine file, there may be specific functionality which we decide to fold into RolemodelRails. Instead of everything being a generator and all code living in each project code base, we might opt to treat some features as provided by the engine and therefor upgradable via bundle update.
Pre-merge checklist
bin/bump_versionorbin/bump_version --patch