Skip to content

Commit bdb7307

Browse files
justin808claude
andcommitted
Refine documentation and test structure per code review
Addresses additional code review suggestions: 1. Edge Case Documentation: - Added @note about manual ENV variable setting - Documented use case for debugging/setup scenarios - Warned against bypassing validation in production 2. Improved Test Structure: - Refactored precedence tests to use nested contexts - Changed structure: "with other skip conditions also present" -> "when package.json exists and ARGV indicates generator" -> "when package.json is missing" - More descriptive test names focusing on behavior - Better organization showing ENV precedence clearly 3. Simplified Comments: - Reduced redundancy in install_generator.rb ensure block - Changed from 2 lines to 1 concise line - Removed redundant statement about concurrent generators Changes maintain all functionality while improving readability and addressing edge cases in documentation. All specs pass (21 examples, 0 failures) RuboCop clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 1a75fcb commit bdb7307

File tree

3 files changed

+37
-30
lines changed

3 files changed

+37
-30
lines changed

lib/generators/react_on_rails/install_generator.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ def run_generators
7373
GeneratorMessages.add_error(error)
7474
end
7575
ensure
76-
# Always clean up ENV variable to avoid affecting subsequent processes
77-
# This is safe even if concurrent generators run in separate processes
76+
# Always clean up ENV variable, even if generator fails
7877
ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
7978
print_generator_messages
8079
end

lib/react_on_rails/engine.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ class Engine < ::Rails::Engine
2929
# run in a single process, so concurrent execution is not a concern. If running
3030
# generators concurrently (e.g., in parallel tests), ensure tests run in separate
3131
# processes to avoid ENV variable conflicts.
32+
#
33+
# @note Manual ENV Setting: While this ENV variable is designed to be set by generators,
34+
# users can manually set it (e.g., `REACT_ON_RAILS_SKIP_VALIDATION=true rails server`)
35+
# to bypass validation. This should only be done temporarily during debugging or
36+
# setup scenarios. The validation helps catch version mismatches early, so bypassing
37+
# it in production is not recommended.
3238
def self.skip_version_validation?
3339
# Skip if explicitly disabled via environment variable (set by generators)
3440
# Using ENV variable instead of ARGV because Rails can modify/clear ARGV during

spec/react_on_rails/engine_spec.rb

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32,40 +32,42 @@ module ReactOnRails
3232
.with("[React on Rails] Skipping validation - disabled via environment variable")
3333
end
3434

35-
context "with package.json present and ARGV set to generator" do
36-
before do
37-
allow(File).to receive(:exist?).with(package_json_path).and_return(true)
38-
stub_const("ARGV", ["generate", "react_on_rails:install"])
39-
end
35+
context "with other skip conditions also present" do
36+
context "when package.json exists and ARGV indicates generator" do
37+
before do
38+
allow(File).to receive(:exist?).with(package_json_path).and_return(true)
39+
stub_const("ARGV", ["generate", "react_on_rails:install"])
40+
end
4041

41-
it "returns true (ENV takes precedence over other conditions)" do
42-
expect(described_class.skip_version_validation?).to be true
43-
end
42+
it "prioritizes ENV over ARGV check" do
43+
expect(described_class.skip_version_validation?).to be true
44+
end
4445

45-
it "logs ENV variable message, not ARGV message" do
46-
described_class.skip_version_validation?
47-
expect(Rails.logger).to have_received(:debug)
48-
.with("[React on Rails] Skipping validation - disabled via environment variable")
49-
expect(Rails.logger).not_to have_received(:debug)
50-
.with("[React on Rails] Skipping validation during generator runtime")
46+
it "short-circuits before checking ARGV" do
47+
described_class.skip_version_validation?
48+
expect(Rails.logger).to have_received(:debug)
49+
.with("[React on Rails] Skipping validation - disabled via environment variable")
50+
expect(Rails.logger).not_to have_received(:debug)
51+
.with("[React on Rails] Skipping validation during generator runtime")
52+
end
5153
end
52-
end
5354

54-
context "with package.json missing" do
55-
before do
56-
allow(File).to receive(:exist?).with(package_json_path).and_return(false)
57-
end
55+
context "when package.json is missing" do
56+
before do
57+
allow(File).to receive(:exist?).with(package_json_path).and_return(false)
58+
end
5859

59-
it "returns true (ENV takes precedence over package.json check)" do
60-
expect(described_class.skip_version_validation?).to be true
61-
end
60+
it "prioritizes ENV over package.json check" do
61+
expect(described_class.skip_version_validation?).to be true
62+
end
6263

63-
it "logs ENV variable message, not package.json message" do
64-
described_class.skip_version_validation?
65-
expect(Rails.logger).to have_received(:debug)
66-
.with("[React on Rails] Skipping validation - disabled via environment variable")
67-
expect(Rails.logger).not_to have_received(:debug)
68-
.with("[React on Rails] Skipping validation - package.json not found")
64+
it "short-circuits before checking package.json" do
65+
described_class.skip_version_validation?
66+
expect(Rails.logger).to have_received(:debug)
67+
.with("[React on Rails] Skipping validation - disabled via environment variable")
68+
expect(Rails.logger).not_to have_received(:debug)
69+
.with("[React on Rails] Skipping validation - package.json not found")
70+
end
6971
end
7072
end
7173
end

0 commit comments

Comments
 (0)