feat: add remove commands for mods and tools collections (#12)#13
feat: add remove commands for mods and tools collections (#12)#13
Conversation
Adds direct removal commands for the mods and tools collections, along with cascade delete functionality for repository removal. - Add 'imt remove mod' to delete entries from mods collection - Add 'imt remove tool' to delete entries from tools collection - Enhance 'imt remove repos' with --cascade flag (enabled by default) to automatically remove associated modinfo, toolinfo, mods, and tools - Include comprehensive test coverage for all new functionality - Update documentation (README, CHANGELOG) for new commands - Bump version to 2.4.0 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds direct removal commands for individual mods and tools, and enhances repository removal with cascade delete functionality that automatically cleans up associated entries across collections.
Changes:
- New
imt remove modandimt remove toolcommands for deleting entries from their respective collections - Enhanced
imt remove reposwith--cascadeflag (enabled by default) to automatically remove associated modinfo, toolinfo, mods, and tools - Comprehensive test coverage for all new functionality with 290 passing tests
- Updated documentation in README and CHANGELOG, version bumped to 2.4.0
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/icarus/mod/cli/remove.rb | Implements new mod and tool removal commands, adds cascade_delete_repo method with URL-based entity matching and cleanup logic |
| spec/icarus/mod/cli/remove_spec.rb | Adds comprehensive test coverage for new commands including cascade delete scenarios, error cases, and dry-run functionality |
| lib/icarus/mod/version.rb | Version bump from 2.3.0 to 2.4.0 |
| README.md | Documents new commands and options including cascade flag behavior |
| CHANGELOG.md | Records changes for v2.4.0 release |
| Gemfile.lock | Updates lock file to reflect new version |
| if options[:dry_run] | ||
| puts Paint["Dry run; no changes will be made", :yellow] | ||
| puts "Would remove:" | ||
| puts " - Repository: #{repo_name}" | ||
| puts " - Modinfo URLs: #{modinfo_urls.size}" | ||
| puts " - Toolinfo URLs: #{toolinfo_urls.size}" | ||
| return | ||
| end |
There was a problem hiding this comment.
The dry-run output for cascade delete only shows the count of modinfo and toolinfo URLs that would be removed, but doesn't show which specific mods and tools would be deleted. Since the actual deletion fetches data from URLs to determine which entities to remove, users running a dry-run won't see the full impact. Consider fetching and displaying the list of entities that would be deleted during a dry-run to provide more comprehensive information.
| else | ||
| warn Paint["Failed to remove repository: #{repo_name}", :red] | ||
| exit 1 | ||
| end |
There was a problem hiding this comment.
The cascade delete operation performs multiple deletions but doesn't invalidate the cached collections in the Firestore instance. After deleting mods and tools through firestore.delete(), the cached @mods and @tools instance variables in the Firestore class will still contain the deleted entities. This could lead to stale data if the same Firestore instance is reused. Consider reinitializing the Firestore instance after cascade operations or ensuring cache invalidation occurs.
| end | |
| end | |
| # Invalidate cached Firestore collections after cascade delete | |
| $firestore = Firestore.new |
| modinfo_urls.each do |url| | ||
| puts Paint[" Removing modinfo: #{url}", :black] if verbose? | ||
| firestore.delete(:modinfo, url) | ||
|
|
||
| # Find and delete associated mods | ||
| delete_entities_from_url(url, :mod) | ||
| end | ||
|
|
||
| # Delete toolinfo URLs and their associated tools | ||
| toolinfo_urls.each do |url| | ||
| puts Paint[" Removing toolinfo: #{url}", :black] if verbose? | ||
| firestore.delete(:toolinfo, url) | ||
|
|
||
| # Find and delete associated tools | ||
| delete_entities_from_url(url, :tool) | ||
| end |
There was a problem hiding this comment.
The cascade delete operations don't check the return values of individual firestore.delete() calls. If a deletion fails partway through the cascade (e.g., deleting a modinfo URL succeeds but deleting an associated mod fails), the operation continues, potentially leaving the database in an inconsistent state. Consider adding error handling to track failures and either roll back changes or at least report which deletions failed.
This commit addresses all 6 review comments from GitHub Copilot on PR #13: 1. Enhanced dry-run preview - Now shows detailed list of all entities that would be deleted (mods, tools, URLs) instead of just counts 2. Improved error handling - Use specific exception types instead of broad StandardError, track failed URL fetches with detailed reporting 3. Precise URL matching - Use regex with word boundaries to prevent false matches (e.g., "owner/repo" no longer matches "owner/repo-fork") 4. Firestore cache invalidation - Delete operations now properly invalidate cached @mods and @tools collections 5. Multiple entity handling - When multiple entities match same name+author, all are deleted (not just first match) 6. Delete failure tracking - Track and report all delete operation failures with comprehensive summary Testing: - All 297 tests passing - Added comprehensive test coverage for all fixes - Updated existing test to use SocketError instead of generic StandardError Documentation: - Updated CHANGELOG.md with detailed notes about improvements Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
Resolves #12
Resolves #5
Adds direct removal commands for the
modsandtoolscollections, along with cascade delete functionality for repository removal.Changes
imt remove mod <MOD_ID>to delete entries from mods collectionimt remove tool <TOOL_ID>to delete entries from tools collectionimt remove reposwith--cascadeflag (enabled by default) to automatically remove associated modinfo, toolinfo, mods, and toolsTest Plan
🤖 Generated with Claude Code