Skip to content

Commit eb16495

Browse files
justin808claude
andauthored
Address code review feedback for validation skip documentation (#1926)
Implements three improvements based on code review feedback: 1. Enhanced documentation with concrete testing example - Added @example block showing how to clean up ENV in parallel tests - Makes the thread safety guidance more actionable for developers 2. Improved test assertion specificity - Added File.exist? spy assertion to verify short-circuit behavior - Confirms ENV check truly returns early without file system access 3. Clarified ensure block cleanup order importance - Added critical comment explaining why ENV cleanup must execute first - Prevents ENV pollution if print_generator_messages raises exception - Documents the intentional ordering for future maintainers All tests pass and RuboCop reports no offenses. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
1 parent 847edcd commit eb16495

File tree

3 files changed

+14
-0
lines changed

3 files changed

+14
-0
lines changed

lib/generators/react_on_rails/install_generator.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ def run_generators
7474
end
7575
ensure
7676
# Always clean up ENV variable, even if generator fails
77+
# CRITICAL: ENV cleanup must come first to ensure it executes even if
78+
# print_generator_messages raises an exception. This prevents ENV pollution
79+
# that could affect subsequent processes.
7780
ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
7881
print_generator_messages
7982
end

lib/react_on_rails/engine.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ class Engine < ::Rails::Engine
3030
# generators concurrently (e.g., in parallel tests), ensure tests run in separate
3131
# processes to avoid ENV variable conflicts.
3232
#
33+
# @example Testing with parallel processes
34+
# # In RSpec configuration:
35+
# config.before(:each) do |example|
36+
# ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
37+
# end
38+
#
3339
# @note Manual ENV Setting: While this ENV variable is designed to be set by generators,
3440
# users can manually set it (e.g., `REACT_ON_RAILS_SKIP_VALIDATION=true rails server`)
3541
# to bypass validation. This should only be done temporarily during debugging or

spec/react_on_rails/engine_spec.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ module ReactOnRails
5050
expect(Rails.logger).not_to have_received(:debug)
5151
.with("[React on Rails] Skipping validation during generator runtime")
5252
end
53+
54+
it "short-circuits before checking File.exist?" do
55+
described_class.skip_version_validation?
56+
expect(File).not_to have_received(:exist?)
57+
end
5358
end
5459

5560
context "when package.json is missing" do

0 commit comments

Comments
 (0)