Guarded rm#382
Conversation
this is for later reuse in rm
This is what happens if you trust AI.
check `ok_to_empty` attrib rather than the `UserIntention`
There was a problem hiding this comment.
Pull request overview
Adds an interactive “guard” confirmation flow reused by both trash-empty and trash-rm, and prevents the “no items to delete” case from incorrectly blocking on user input.
Changes:
- Introduce a reusable
trashcli.guardconfirmation layer (Guard,User, reply parsing, TTY detection). - Update
trash-emptyto skip prompting when there’s nothing to empty (prints message and exits). - Update
trash-rmto display matched items and ask for confirmation before deleting.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| trashcli/rm/rm_cmd.py | Collects matching trashed entries, prompts user via Guard, and deletes on confirmation |
| trashcli/rm/prepare_output_message.py | Formats the confirmation prompt for trash-rm |
| trashcli/guard/user.py | Generalizes user confirmation method (confirm) |
| trashcli/guard/parse_reply.py | Adds shared reply parsing (y/default no) |
| trashcli/guard/is_input_interactive.py | Adds shared stdin-TTY detection |
| trashcli/guard/guard.py | Reuses guard logic with generalized User.confirm() |
| trashcli/empty/empty_cmd.py | Switches to shared is_input_interactive |
| trashcli/empty/empty_action.py | Avoids prompting when there are no trash dirs |
| tests/test_empty/components/test_user.py | Updates import + method name to User.confirm() |
| tests/test_empty/components/test_parse_reply.py | Updates import to shared parse_reply |
| tests/test_empty/components/test_guard.py | Updates mock spec + method name to confirm |
Comments suppressed due to low confidence (1)
trashcli/guard/guard.py:19
- The guard logic is now reused beyond
trash-empty(e.g.,trash-rmpasses tuples of(original_location, info_file)), but this module still hard-codesTrashDirtypes andtrash_dirsnaming inUserIntentionand method parameters. This mismatch makes the API misleading and can confuse future refactors/type checking. Consider makingGuardgeneric (e.g.,Iterable[T] -> UserIntention[T]) or renaming the parameter/field to something likeitems_to_confirmand loosening the type fromTrashDir.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if files_to_remove: # skip asking the user if there is nothing to delete; avoids being stuck | ||
| user = User(prepare_output_message, RealInput(), parse_reply) | ||
| guard = Guard(user) | ||
| if guard.ask_the_user(is_input_interactive(), files_to_remove).ok_to_empty: | ||
| for file in files_to_remove: |
There was a problem hiding this comment.
This change hard-codes interactive detection (is_input_interactive()) and uses RealInput() directly inside run(). That makes behavior depend on whether stdin is a TTY (tests or callers running in a TTY can hang waiting for input), and it’s difficult to unit test the yes/no paths without patching global stdin. Consider injecting an Input implementation (like HardCodedInput) and/or parsing -i/-f flags similarly to trash-empty, so tests and non-interactive callers can reliably control confirmation behavior.
| if files_to_remove: # skip asking the user if there is nothing to delete; avoids being stuck | ||
| user = User(prepare_output_message, RealInput(), parse_reply) | ||
| guard = Guard(user) | ||
| if guard.ask_the_user(is_input_interactive(), files_to_remove).ok_to_empty: | ||
| for file in files_to_remove: | ||
| trashcan.delete_trash_info_and_backup_copy(file[1]) | ||
| else: | ||
| print('No files / directories to be removed in {}'.format(path)) |
There was a problem hiding this comment.
New guarded-deletion behavior is introduced here, but there are no tests exercising the interactive confirmation branches (user answers yes vs no), nor the “no matches” path. Since tests/test_rm/ already has command-level and integration tests for RmCmd, it would be good to extend them to cover the new prompt flow and ensure it doesn’t regress or hang (especially when stdin is a TTY).
| else: | ||
| print('No trash directories to empty.') |
There was a problem hiding this comment.
This branch prints directly to stdout via print(), bypassing the Console abstraction that EmptyCmd wires up (and which is used elsewhere for output). This makes it harder to redirect/capture output consistently (e.g., when EmptyCmd is used programmatically with a custom out). Prefer emitting this message through console.out (or add a dedicated Console.print_info() method) instead of using print().
|
Will close #383. (I created this issue in case the maintainer(s) only check(s) for new issues.) |
Overview
This pull request guards
trash-rmby showing files / dirs to be removed, and ask for confirmation before removing. It also fix the issue found in development thattrash-emptywill display “No trash directories to empty.” and start a new line, waiting for user input, as if waiting for user to answer to y/N. Now, bothtrash-emptyandtrash-rmexits after displaying a message indicating nothing is to be done.When implementing this feature, several modules were separated from the 'empty' subpackage, forming a 'guard' subpackage, and reused in 'rm'.
Next steps
trash-empty)guardmodule (and also inrmand the like if possible; mypyrightactually complains a lot about the codebase)guardpackage so they are more generic and nottrash-empty-related./for trashed directories' name. This hints the user that it contains files in it, not listed bytrash-cli.