Skip to content

Conversation

thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Aug 25, 2024

Motivation / Background

Fixes #52704

This Pull Request has been created because the devcontainer generator with the --dev option fails (rails g devcontainer --dev). The app generator --dev works just fine.

Detail

This Pull request moves the Rails::Generators::RAILS_DEV_PATH definition to Rails::Generators module rather than adding it to Rails::Generators in AppGenerator definition. It also creates a DEV_RAILS_PATH constant in railties/test/generators/generators_test_helper.rb, so that it can be used consistently in tests.

Additional information

  • I added a generator test file, it seems the generator is not tested for the rails generate usage, though it is well tested via app generator. I added tests for the dev option behavior only. I can work on full specs, but will keep it separate from this.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.

@rails-bot rails-bot bot added the railties label Aug 25, 2024
@thejonroberts thejonroberts force-pushed the devcontainer-generator-dev-flag-error branch from 9f32df7 to 673feaf Compare August 25, 2024 14:58
@thejonroberts thejonroberts force-pushed the devcontainer-generator-dev-flag-error branch from 673feaf to 2f27b66 Compare August 25, 2024 15:28
@thejonroberts
Copy link
Contributor Author

Tests for the rest of the generator functionality should probably be added to the new test file?

@thejonroberts thejonroberts force-pushed the devcontainer-generator-dev-flag-error branch from 2f27b66 to 3ad802a Compare August 25, 2024 20:18
@jeromedalbert
Copy link
Contributor

jeromedalbert commented Aug 25, 2024

Tests for the rest of the generator functionality should probably be added to the new test file?

The devcontainer generator functionality looks like it has been pretty extensively (albeit indirectly) tested in app_generator_test.rb, since AppGenerator calls DevcontainerGenerator when the --devcontainer option is supplied.

If you are suggesting that you want to move (or copy/share) those tests over to your new test file, to me (with limited knowledge of this codebase's best practices) the PR looks good as is. There are just enough tests for some sanity checks and testing that the bug has been fixed.

@thejonroberts thejonroberts force-pushed the devcontainer-generator-dev-flag-error branch 2 times, most recently from 1aca997 to dfc38cb Compare August 26, 2024 04:33
@thejonroberts thejonroberts force-pushed the devcontainer-generator-dev-flag-error branch 3 times, most recently from 3f89a06 to 3681358 Compare August 27, 2024 06:20
@andrewn617
Copy link
Member

This looks good to me. Thanks =)

@andrewn617 andrewn617 added the ready PRs ready to merge label Aug 27, 2024
@rafaelfranca rafaelfranca force-pushed the devcontainer-generator-dev-flag-error branch from 3681358 to 7accd04 Compare August 27, 2024 18:30
Devcontainer generator errors with uninitialized constant
Rails::Generators::RAILS_DEV_PATH, which is defined with
AppGenerator in .../generators/rails/app/app_generator.rb.
Move definition to rails/generators.

Co-authored-by: Jerome Dalbert <jerome.dalbert@gmail.com>
@rafaelfranca rafaelfranca force-pushed the devcontainer-generator-dev-flag-error branch from 7accd04 to cd6e5d6 Compare August 27, 2024 18:34
@rafaelfranca rafaelfranca merged commit 63a532e into rails:main Aug 27, 2024
3 checks passed
rafaelfranca added a commit that referenced this pull request Aug 27, 2024
…ev-flag-error

Fix Devcontainer generator with --dev option path error
@thejonroberts thejonroberts deleted the devcontainer-generator-dev-flag-error branch August 30, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
railties ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Devcontainer generator with --dev flag unitialized constant.
5 participants