Create swap-shakacode-deps gem structure and documentation #54
Conversation
- Add comprehensive implementation plan outlining architecture and migration strategy - Create Ruby gem structure with gemspec, Rakefile, and standard directories - Add detailed README with installation instructions and usage examples - Include LICENSE (MIT), CHANGELOG, and .rubocop.yml configuration - Create CLI module with full option parsing matching current bin/swap-deps - Add USAGE_EXAMPLES.md demonstrating real-world workflows - Define error classes and module structure for the gem This provides the foundation for a globally installable tool that brings swap-deps functionality to any Shakacode project, not just demos. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds a new Ruby gem project swap-shakacode-deps with CLI and executable, core swapper components (Swapper, GemSwapper, NpmSwapper), managers (BackupManager, CacheManager, WatchManager), config and parsing utilities, error types, versioning, docs, tooling (Rake, RuboCop), and scaffolding (gemspec, Gemfile, LICENSE, changelog, examples). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI
participant Sw as Swapper
participant Cache as CacheManager
participant Watch as WatchManager
User->>CLI: run!
CLI->>CLI: parse_options!
alt cache/status command
CLI->>Cache: show_info / clean(...)
Cache-->>CLI: info/ack
CLI-->>User: output
else watch command
CLI->>Watch: list_processes / kill_processes
Watch-->>CLI: pids/ack
CLI-->>User: output
else apply/restore
CLI->>Sw: load_config/apply_from_config / restore!
Sw-->>CLI: result
CLI-->>User: output
else swap (local/github)
CLI->>Sw: swap!(gem_paths, github_repos, options)
Sw->>Cache: maybe update cache
Sw->>CLI: result
CLI-->>User: output
end
note over CLI,Sw: Errors mapped to Validation/Config/Backup/Cache errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: swap-shakacode-deps Gem Structure and DocumentationThank you for this well-organized PR! This is an excellent foundation for creating a globally installable dependency swapping tool. Here's my comprehensive review: ✅ Strengths
🐛 Potential IssuesCritical Issues
Medium Priority Issues
🔒 Security ConsiderationsLow Risk Issues
🚀 Performance Considerations
📝 Test CoverageCritical Gap
💡 Code Quality Suggestions
📋 Documentation
✨ Best Practices AdherenceExcellent Work On:
🎯 Summary and RecommendationsThis PR provides an excellent foundation for the swap-shakacode-deps gem. The documentation and structure are solid, making it easy to understand the intended functionality. Before Merging:
For Next PR:
Overall Assessment:Excellent work! This PR demonstrates thoughtful planning and good Ruby practices. The transition from a project-specific script to a global gem is well-designed. I'm excited to see the implementation in future PRs. Review completed with: Code quality ✓ | Potential bugs ✓ | Performance ✓ | Security ✓ | Test coverage ✓ 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
swap-shakacode-deps/swap-shakacode-deps.gemspec (1)
26-27: Consider refining the file glob pattern.The
File::FNM_DOTMATCHflag will include hidden files (like.DS_Storeor.git*) in the gem package. Consider using a more explicit pattern or adding exclusions.Example refinement:
- spec.files = Dir.glob('{bin,lib}/**/*', File::FNM_DOTMATCH) + - %w[README.md LICENSE CHANGELOG.md] + spec.files = Dir.glob('{bin,lib}/**/*') + + %w[README.md LICENSE CHANGELOG.md]Or use
git ls-filesfor precise control:- spec.files = Dir.glob('{bin,lib}/**/*', File::FNM_DOTMATCH) + - %w[README.md LICENSE CHANGELOG.md] + spec.files = Dir.chdir(File.expand_path(__dir__)) do + `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features)/}) } + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
swap-shakacode-deps/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
swap-shakacode-deps-implementation-plan.md(1 hunks)swap-shakacode-deps/.rubocop.yml(1 hunks)swap-shakacode-deps/CHANGELOG.md(1 hunks)swap-shakacode-deps/Gemfile(1 hunks)swap-shakacode-deps/LICENSE(1 hunks)swap-shakacode-deps/README.md(1 hunks)swap-shakacode-deps/Rakefile(1 hunks)swap-shakacode-deps/USAGE_EXAMPLES.md(1 hunks)swap-shakacode-deps/bin/swap-shakacode-deps(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/cli.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/error.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/version.rb(1 hunks)swap-shakacode-deps/swap-shakacode-deps.gemspec(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
swap-shakacode-deps/lib/swap_shakacode_deps/cli.rb (1)
lib/demo_scripts/gem_swapper.rb (3)
swap!(42-59)restore!(61-77)show_status(137-143)
🪛 markdownlint-cli2 (0.18.1)
swap-shakacode-deps/README.md
276-276: Bare URL used
(MD034, no-bare-urls)
swap-shakacode-deps-implementation-plan.md
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (7)
swap-shakacode-deps/LICENSE (1)
1-21: LGTM!Standard MIT License with appropriate copyright information.
swap-shakacode-deps/Gemfile (1)
1-6: LGTM!Standard Gemfile structure that properly delegates dependency management to the gemspec.
swap-shakacode-deps/bin/swap-shakacode-deps (1)
1-7: LGTM!Clean executable entry point with proper shebang and minimal bootstrap code. Ensure the file has executable permissions (
chmod +x) when packaged.swap-shakacode-deps/lib/swap_shakacode_deps/version.rb (1)
1-5: LGTM!Standard version file following Ruby gem conventions.
swap-shakacode-deps/CHANGELOG.md (1)
1-34: Well-structured changelog.The changelog follows Keep a Changelog format and comprehensively documents the initial feature set.
swap-shakacode-deps/Rakefile (1)
1-13: LGTM!Well-structured Rakefile with standard testing and linting tasks. The composite
checktask is a good practice for running all quality checks.swap-shakacode-deps/swap-shakacode-deps.gemspec (1)
1-44: Well-structured gemspec with comprehensive metadata.The gemspec follows Ruby gem conventions with proper metadata, dependencies, and file specifications. The MFA requirement for RubyGems is a good security practice.
| ``` | ||
| swap-shakacode-deps/ | ||
| ├── bin/ | ||
| │ └── swap-shakacode-deps # Executable | ||
| ├── lib/ | ||
| │ ├── swap_shakacode_deps.rb # Main entry | ||
| │ ├── swap_shakacode_deps/ | ||
| │ │ ├── version.rb | ||
| │ │ ├── cli.rb # CLI parser | ||
| │ │ ├── swapper.rb # Core swapping logic | ||
| │ │ ├── gem_swapper.rb # Gemfile manipulation | ||
| │ │ ├── npm_swapper.rb # package.json manipulation | ||
| │ │ ├── github_handler.rb # GitHub repo management | ||
| │ │ ├── cache_manager.rb # Cache operations | ||
| │ │ ├── watch_manager.rb # Watch process management | ||
| │ │ ├── backup_manager.rb # Backup/restore logic | ||
| │ │ └── config_loader.rb # YAML config handling | ||
| ├── spec/ # Tests | ||
| ├── README.md | ||
| ├── CHANGELOG.md | ||
| ├── LICENSE | ||
| ├── Gemfile | ||
| ├── Rakefile | ||
| └── swap-shakacode-deps.gemspec | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The fenced code block representing the file structure should have a language identifier for proper rendering and syntax highlighting.
Apply this diff:
-```
+```plaintext
swap-shakacode-deps/
├── bin/
│ └── swap-shakacode-deps # Executable🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In swap-shakacode-deps-implementation-plan.md around lines 54 to 78, the fenced
code block showing the directory tree lacks a language identifier; update the
opening fence from ``` to ```plaintext so the block starts with ```plaintext and
leave the rest of the block unchanged to enable proper rendering/syntax
highlighting.
swap-shakacode-deps/CHANGELOG.md
Outdated
|
|
||
| ## [Unreleased] | ||
|
|
||
| ## [0.1.0] - 2024-01-XX |
There was a problem hiding this comment.
Update the placeholder date before release.
The release date contains a placeholder "2024-01-XX" that should be updated to the actual release date.
🤖 Prompt for AI Agents
In swap-shakacode-deps/CHANGELOG.md around line 10, the release entry still uses
the placeholder date "2024-01-XX"; replace that placeholder with the actual
release date (e.g., "2024-01-15") so the changelog reflects the correct
publication date and commit the updated file.
| def find_config_file | ||
| if @options[:target_path] | ||
| config_path = File.join(@options[:target_path], CONFIG_FILE) | ||
| return config_path if File.exist?(config_path) | ||
| elsif File.exist?(CONFIG_FILE) | ||
| return CONFIG_FILE | ||
| end | ||
| nil | ||
| end |
There was a problem hiding this comment.
Fix config discovery to honor --path and home configs
File.join leaves ~ untouched, so --apply --path ~/app never finds the config even when it exists. We also never consult ~/.swap-deps.yml, yet the docs rely on that location. Both gaps break common workflows.
Please normalize the candidate paths with File.expand_path and include the home-directory fallback:
- if @options[:target_path]
- config_path = File.join(@options[:target_path], CONFIG_FILE)
- return config_path if File.exist?(config_path)
- elsif File.exist?(CONFIG_FILE)
- return CONFIG_FILE
- end
- nil
+ candidates = []
+ candidates << File.expand_path(CONFIG_FILE, @options[:target_path]) if @options[:target_path]
+ candidates << File.expand_path(CONFIG_FILE, Dir.pwd)
+ candidates << File.expand_path(CONFIG_FILE, Dir.home)
+
+ candidates.find { |path| File.exist?(path) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def find_config_file | |
| if @options[:target_path] | |
| config_path = File.join(@options[:target_path], CONFIG_FILE) | |
| return config_path if File.exist?(config_path) | |
| elsif File.exist?(CONFIG_FILE) | |
| return CONFIG_FILE | |
| end | |
| nil | |
| end | |
| def find_config_file | |
| candidates = [] | |
| candidates << File.expand_path(CONFIG_FILE, @options[:target_path]) if @options[:target_path] | |
| candidates << File.expand_path(CONFIG_FILE, Dir.pwd) | |
| candidates << File.expand_path(CONFIG_FILE, Dir.home) | |
| candidates.find { |path| File.exist?(path) } | |
| end |
🤖 Prompt for AI Agents
In swap-shakacode-deps/lib/swap_shakacode_deps/cli.rb around lines 315 to 323,
config discovery fails when --path contains ~ and it never checks the user's
home config; normalize candidate paths with File.expand_path for
@options[:target_path] before joining with CONFIG_FILE (so ~ is expanded), still
fall back to checking the local CONFIG_FILE if no --path, and if neither is
found also check File.expand_path('~/.swap-deps.yml') and return that path when
present.
| # Create a config file in your home directory | ||
| cat > ~/.swap-deps.yml << EOF | ||
| gems: | ||
| shakapacker: ~/dev/shakapacker | ||
| react_on_rails: ~/dev/react_on_rails | ||
| EOF | ||
|
|
||
| # Apply to any project | ||
| cd ~/projects/app1 | ||
| swap-shakacode-deps --apply | ||
|
|
||
| cd ~/projects/app2 | ||
| swap-shakacode-deps --apply | ||
| ``` |
There was a problem hiding this comment.
Home-directory config example currently fails
The CLI only looks for .swap-deps.yml in the target directory or the current working directory, so this workflow against ~/.swap-deps.yml will always raise “Config file not found.” Please either revise the docs or (preferably) extend find_config_file to search the user’s home directory so the documented behavior works.
Based on thorough code review, made the following improvements: ## Added Files - .gitignore: Standard Ruby gem gitignore with IDE and OS files - .swap-deps.yml.example: Example configuration file with documentation - Stub implementation modules for all required classes: - BackupManager: File backup/restore operations - CacheManager: GitHub repository cache management - WatchManager: NPM watch process management - GemSwapper: Gemfile dependency manipulation - NpmSwapper: package.json dependency manipulation - ConfigLoader: YAML configuration file handling - GitHubSpecParser: GitHub repo spec parsing - Swapper: Main orchestrator class ## Fixed Issues - Removed yaml gem dependency (YAML is in Ruby stdlib) - Updated CHANGELOG to mark version as [Unreleased] - Removed Gemfile.lock (shouldn't be committed for libraries) - Added stub implementations with TODOs for next iteration - All modules show helpful "not yet implemented" messages - Fixed RuboCop offenses (unused params, code style) - Added RuboCop disables for acceptable complexity metrics ## Implementation Notes Each module contains: - Clear class/module documentation - TODO comments referencing source implementation - Basic structure and method signatures - NotImplementedError or informative messages for users This allows the gem to load and run, displaying helpful messages about upcoming functionality rather than crashing with missing files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
✅ Code Review Feedback AddressedThank you for the thorough code review! I've addressed all the critical and medium priority issues: Fixed Issues:
Implementation Structure:Each stub module includes:
Next Steps:The gem can now be loaded and run without errors. Users will see helpful messages about upcoming functionality. The next PR will focus on extracting and implementing the actual swapping logic from the existing This approach allows for:
|
Code Review - PR #54: Create swap-shakacode-deps gem structure and documentationThank you for this comprehensive PR! This is a well-structured foundation for extracting the swap-deps functionality into a standalone gem. Below is my detailed review: 🎯 Overall AssessmentThis PR provides an excellent skeleton structure and planning documentation for the new gem. The approach of creating the structure first before implementing the logic is sound and makes the architecture review straightforward. Summary: ✅ Approved with suggestions - The structure is solid, but there are areas for improvement before implementation begins. ✅ Strengths1. Excellent Documentation
2. Well-Organized Structure
3. CLI Design
4. Configuration Management
🔍 Issues & Concerns1. Critical: Missing Implementation (Expected)All core modules contain
Recommendation: Create GitHub issues for each module implementation to track progress in Phase 2. 2. Security ConsiderationsFile Path HandlingThe CLI accepts arbitrary file paths without validation: # swap-shakacode-deps/lib/swap_shakacode_deps/cli.rb:80-90
opts.on('--shakapacker PATH', 'Path to local shakapacker repository') do |path|
@gem_paths['shakapacker'] = path
endRecommendations:
GitHub Repository Validation
Issues:
Recommendations:
Shell Command Execution (Future Risk)Since this tool will eventually run
3. Code Quality IssuesRuboCop ViolationsMultiple RuboCop disables without justification:
Recommendation: Consider refactoring to avoid needing these disables. The CLI class could be split:
Inconsistent Error Handling
Recommendation: Document which errors are expected vs unexpected, or add logging. 4. Missing Test CoverageNo test files are included in this PR. While the PR description mentions tests will come in Phase 2, consider:
Recommendation: Add skeleton test structure with spec_helper.rb 5. Dependency ConcernsGemspec Dependencies
spec.add_dependency 'json', '~> 2.0'Questions:
6. Configuration File Location
Recommendation: Consider a search order like:
7. Documentation GapsMissing Sections
Unclear Behavior
🎨 Best Practices & Suggestions1. Add Version CheckConsider adding a check for compatible Ruby versions at runtime, not just in gemspec. 2. Better User FeedbackWhen implemented, ensure clear progress indicators:
3. IdempotencyDocument whether operations are idempotent:
4. Integration with BundlerConsider whether this should integrate with 5. Atomic Operations
📊 Performance Considerations1. GitHub Cache Strategy
Recommendations:
2. Recursive Processing
Recommendations:
🧪 Test Coverage RecommendationsWhen implementing tests in Phase 2:
📋 Checklist for Next PhaseBefore merging Phase 2 implementation:
🎉 ConclusionThis is a well-planned and well-documented PR that sets up an excellent foundation for the swap-shakacode-deps gem. The architecture is sound, the documentation is thorough, and the separation of concerns is clean. The main areas needing attention are:
Once the implementation is complete with proper security measures and tests, this will be a valuable tool for the Shakacode ecosystem. Recommendation: ✅ Merge this PR to establish the structure, then create detailed issues for Phase 2 implementation with focus on security and testing. Review conducted by Claude Code 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
swap-shakacode-deps/swap-shakacode-deps.gemspec (1)
25-27: Filter out directories fromspec.files.
Dir.globwithFile::FNM_DOTMATCHpulls in./..entries and directories, sogem buildwill warn that those aren’t files. Drop the directories when constructingspec.files.- spec.files = Dir.glob('{bin,lib}/**/*', File::FNM_DOTMATCH) + - %w[README.md LICENSE CHANGELOG.md] + spec.files = Dir.glob('{bin,lib}/**/*', File::FNM_DOTMATCH) + .reject { |f| File.directory?(f) } + + %w[README.md LICENSE CHANGELOG.md]swap-shakacode-deps/lib/swap_shakacode_deps/gem_swapper.rb (1)
32-39: Consider error handling for bundle install failures.The
systemcall does not check the return value, so bundle install failures will be silent. In dry-run mode this is acceptable, but when actually installing, failures should be detected and reported.Consider this approach:
def run_bundle_install(path) return if @dry_run puts ' Running bundle install...' Dir.chdir(path) do - system('bundle', 'install', '--quiet') + success = system('bundle', 'install', '--quiet') + puts ' ⚠️ bundle install failed' unless success end endAlternatively, for more robust error handling in the full implementation, consider raising an error on failure or returning a status indicator that the caller can check.
swap-shakacode-deps/lib/swap_shakacode_deps/npm_swapper.rb (2)
27-34: Consider error handling for npm install failures.Similar to the bundle install concern, the
systemcall does not check the return value. npm install failures will be silent, which could lead to incomplete or broken dependency states.Apply this pattern:
def run_npm_install(path) return if @dry_run puts ' Running npm install...' Dir.chdir(path) do - system('npm', 'install', '--silent') + success = system('npm', 'install', '--silent') + puts ' ⚠️ npm install failed' unless success end end
37-44: Consider error handling for npm build failures.The build command also lacks error checking. Build failures should be reported to avoid confusion when the swap appears to succeed but the package is not properly built.
def build_npm_package(gem_name, npm_path) return if @dry_run puts " Building #{gem_name}..." Dir.chdir(npm_path) do - system('npm', 'run', 'build') + success = system('npm', 'run', 'build') + puts " ⚠️ Build failed for #{gem_name}" unless success end endswap-shakacode-deps/lib/swap_shakacode_deps/config_loader.rb (1)
27-37: Consider simplifying the validation logic.The validation logic for the github section is correct but uses an inverted guard clause that reduces readability. The pattern
return unless conditionfollowed byraiseis less clear than a direct conditional check.def validate_config(config) raise ConfigError, 'Configuration must be a hash' unless config.is_a?(Hash) # Validate gems section if present raise ConfigError, 'gems section must be a hash' if config['gems'] && !config['gems'].is_a?(Hash) # Validate github section if present - return unless config['github'] && !config['github'].is_a?(Hash) - - raise ConfigError, 'github section must be a hash' + raise ConfigError, 'github section must be a hash' if config['github'] && !config['github'].is_a?(Hash) endswap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb (2)
8-8: ExtractCACHE_DIRto eliminate duplication.The
CACHE_DIRconstant is defined identically in bothWatchManagerandCacheManager. Extract it to a shared module or base class to maintain a single source of truth.Consider creating a shared configuration module:
module SwapShakacodeDeps module Paths CACHE_DIR = File.expand_path('~/.cache/swap-shakacode-deps') end endThen reference it as
Paths::CACHE_DIRin both classes.
17-30: Consider consistent stub implementation approach.The stub methods use two different patterns:
list_processesandkill_processesprint informative messages, whilespawn_watch_processraisesNotImplementedError. For consistency and better user experience, consider using the same approach across all stub methods.If you prefer the informative message approach:
def spawn_watch_process(gem_name, npm_path) - raise NotImplementedError, 'Watch process spawning will be implemented in the next iteration' + puts 'ℹ️ Watch process spawning will be implemented in the next iteration' end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
swap-shakacode-deps/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
swap-shakacode-deps/.gitignore(1 hunks)swap-shakacode-deps/.swap-deps.yml.example(1 hunks)swap-shakacode-deps/CHANGELOG.md(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/backup_manager.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/config_loader.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/gem_swapper.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/github_spec_parser.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/npm_swapper.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb(1 hunks)swap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb(1 hunks)swap-shakacode-deps/swap-shakacode-deps.gemspec(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- swap-shakacode-deps/CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (7)
swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb (2)
swap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb (2)
expand_path(5-41)initialize(12-15)swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
swap-shakacode-deps/lib/swap_shakacode_deps/backup_manager.rb (2)
swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb (1)
initialize(10-13)swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
swap-shakacode-deps/lib/swap_shakacode_deps/gem_swapper.rb (1)
swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (6)
swap-shakacode-deps/lib/swap_shakacode_deps/backup_manager.rb (2)
initialize(5-34)initialize(10-13)swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb (1)
initialize(10-13)swap-shakacode-deps/lib/swap_shakacode_deps/config_loader.rb (3)
initialize(7-38)initialize(10-12)load(15-23)swap-shakacode-deps/lib/swap_shakacode_deps/gem_swapper.rb (2)
initialize(5-40)initialize(8-11)swap-shakacode-deps/lib/swap_shakacode_deps/npm_swapper.rb (2)
initialize(5-45)initialize(8-11)swap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb (1)
initialize(12-15)
swap-shakacode-deps/lib/swap_shakacode_deps/config_loader.rb (1)
swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
swap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb (3)
swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb (2)
expand_path(5-41)initialize(10-13)swap-shakacode-deps/lib/swap_shakacode_deps/npm_swapper.rb (2)
initialize(5-45)initialize(8-11)swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
swap-shakacode-deps/lib/swap_shakacode_deps/npm_swapper.rb (1)
swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (2)
initialize(5-95)initialize(9-24)
🔇 Additional comments (8)
swap-shakacode-deps/.gitignore (1)
1-70: Comprehensive ignore list looks good.The patterns cover typical Ruby gem, tooling, and OS artifacts—nothing concerning here.
swap-shakacode-deps/.swap-deps.yml.example (1)
1-36: LGTM - Well-documented configuration example.The example configuration file is clear, well-structured, and provides helpful inline documentation. The format aligns with the ConfigLoader implementation and includes practical examples for both local paths and GitHub repositories.
swap-shakacode-deps/lib/swap_shakacode_deps/github_spec_parser.rb (1)
5-22: Consider method visibility for module usage.The GitHubSpecParser is defined as a module with instance methods. Depending on the intended usage pattern, you may want to make these methods callable directly on the module using
module_functionor defining them asself.method_name.If these methods are intended to be called as
GitHubSpecParser.parse_github_spec(...)rather than via inclusion/extension, they should be module methods:module GitHubSpecParser # TODO: Extract implementation from demo_scripts/github_spec_parser.rb # Parses GitHub spec: org/repo, org/repo#branch, or org/repo@tag - def parse_github_spec(github_spec) + def self.parse_github_spec(github_spec) raise NotImplementedError, 'GitHub spec parsing will be implemented in the next iteration' end # Validates GitHub repository format (org/repo) - def validate_github_repo(repo) + def self.validate_github_repo(repo) raise NotImplementedError, 'GitHub repo validation will be implemented in the next iteration' end # Validates GitHub branch name according to Git ref naming rules - def validate_github_branch(branch) + def self.validate_github_branch(branch) raise NotImplementedError, 'GitHub branch validation will be implemented in the next iteration' end endAlternatively, if the methods are intended for inclusion/extension (mix-in pattern), the current structure is appropriate. Please verify the intended usage pattern when implementing these methods.
swap-shakacode-deps/lib/swap_shakacode_deps/backup_manager.rb (1)
26-33: LGTM - Utility methods are well-implemented.The
backup_exists?andlist_backupshelper methods are correctly implemented and will work well once the backup/restore operations are added.swap-shakacode-deps/lib/swap_shakacode_deps/cache_manager.rb (1)
31-40: LGTM - Cache path utilities are well-structured.The
github_cache_pathmethod correctly transforms repository and branch names into filesystem-safe slugs, andcache_exists?provides a simple existence check. The implementation aligns with the expected usage from the Swapper class.swap-shakacode-deps/lib/swap_shakacode_deps/swapper.rb (3)
9-24: LGTM - Well-structured orchestrator initialization.The initialization properly sets up all the component managers and swappers, with sensible defaults for options. The use of keyword arguments and splatting options to sub-components creates a flexible and maintainable architecture.
57-75: LGTM - Robust configuration loading with good defaults.The
load_configmethod properly normalizes configuration from YAML, handling both string and symbol keys defensively, and applying sensible defaults (branch: 'main', ref_type: :branch). The defensive key access pattern (info['repo'] || info[:repo]) is good practice for configuration handling.
85-94: LGTM - Clean project discovery logic.The
find_projectsmethod correctly handles both recursive and non-recursive modes, with appropriate file existence checks. The recursive glob pattern will properly discover all Gemfile locations in the directory tree.
| # Checks if a process is running | ||
| def process_running?(pid) | ||
| Process.kill(0, pid) | ||
| true | ||
| rescue Errno::ESRCH | ||
| false | ||
| rescue Errno::EPERM | ||
| true # Process exists but we don't have permission | ||
| end |
There was a problem hiding this comment.
Add input validation for pid parameter.
The method uses Process.kill(0, pid) which expects an integer PID. If pid is nil, non-integer, or invalid, this could raise unexpected errors like TypeError or ArgumentError that aren't handled.
Apply this diff to add validation:
def process_running?(pid)
+ return false if pid.nil? || !pid.is_a?(Integer) || pid <= 0
+
Process.kill(0, pid)
true
rescue Errno::ESRCH
false
rescue Errno::EPERM
true # Process exists but we don't have permission
end🤖 Prompt for AI Agents
In swap-shakacode-deps/lib/swap_shakacode_deps/watch_manager.rb around lines 32
to 40, add input validation for the pid parameter so non-integer, nil, or
otherwise invalid PIDs don't raise unexpected errors: first check that pid is
present and is a positive Integer (or, if you want to accept numeric strings,
normalize with to_i and verify > 0), return false immediately for invalid
values, then call Process.kill(0, pid) as before; also catch TypeError and
ArgumentError in the rescue chain and return false so only the expected
ESRCH/EPERM behaviors remain.
1 participant
Summary
swap-shakacode-depsthat provides dependency swapping functionalitybin/swap-depstoolWhat's Included
Key Benefits
.swap-deps.ymlacross projectsNext Steps
After this PR is merged, the next phase will:
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit