-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Context
From PR #13 code review - nice-to-have code quality improvements.
Improvements
1. Simplify Validation Logic (demo_creator.rb:95-102
)
Current: Multiple conditional checks make method longer.
Current code:
def validate_demo_name!(name)
raise ArgumentError, 'Demo name cannot be empty' if name.nil? || name.strip.empty?
raise ArgumentError, 'Demo name cannot contain slashes' if name.include?('/')
raise ArgumentError, 'Demo name cannot start with . or _' if name.start_with?('.', '_')
return if name.match?(/^[a-zA-Z0-9_-]+$/)
raise ArgumentError, 'Demo name can only contain alphanumeric characters, hyphens, and underscores'
end
Suggested improvement:
def validate_demo_name!(name)
# Valid: not empty, no slashes, doesn't start with . or _, only alphanumeric/hyphens/underscores
unless name =~ /\A(?![._])[a-zA-Z0-9_-]+\z/ && !name.strip.empty?
raise ArgumentError,
'Demo name must be non-empty, not start with . or _, and only contain ' \
'alphanumeric characters, hyphens, and underscores'
end
end
Note: Consider keeping current approach for better error messages. Single regex is more compact but less user-friendly.
2. Extract Duplicate Prerelease Logic (demo_creator.rb:501-502
)
Current: Logic duplicated for determining prerelease status.
Current code:
'shakapacker_prerelease' => @config.shakapacker_version&.start_with?('github:'),
'react_on_rails_prerelease' => @config.react_on_rails_version&.start_with?('github:')
Suggested improvement:
def prerelease_version?(version)
version&.start_with?('github:')
end
# In metadata:
'shakapacker_prerelease' => prerelease_version?(@config.shakapacker_version),
'react_on_rails_prerelease' => prerelease_version?(@config.react_on_rails_version)
3. Simplify .gitignore Conditional (install_generator.rb
)
Current: Conditional logic for handling .gitignore could be simplified.
Current code:
if File.exist?('.gitignore')
append_to_file '.gitignore', gitignore_content
else
create_file '.gitignore', gitignore_content
end
Rails generators already handle this: append_to_file
will create the file if it doesn't exist.
Suggested improvement:
append_to_file '.gitignore', gitignore_content
4. Extract Magic Numbers to Constants
Current locations:
e2e_test_runner.rb:84-86
- Server startup constantse2e_test_runner.rb:54-55
- Port release delay
Recommendation: These are already extracted. No action needed unless adding more magic numbers.
Additional Considerations
Method Length
Some methods are long but readable. Consider:
- Extract methods only if it improves clarity
- Keep cohesive logic together
- Prioritize readability over strict line counts
Comments in Playwright Config
Consider adding comments to playwright.config.ts
explaining:
- Why certain timeouts are set
- What the base URL should be
- Browser configuration rationale
Acceptance Criteria
- Decide on validation approach (clarity vs compactness)
- Extract
prerelease_version?
helper if valuable - Simplify
.gitignore
handling - Add comments to Playwright config
- All tests still pass
- No breaking changes