fix: clarify trashbin expiration CLI help - #63160
Conversation
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
The test should be removed and I’m not sold on the new description/help text.
"Processes deleted files" sounds confusing to me. It deletes expired files, it does not process them?
Per review: "Processes deleted files" was confusing since the command deletes expired files rather than processing them. Description and help now say so directly, and the test that pinned the exact wording is removed.
|
You are right on both counts. "Processes deleted files" was doing no work: the command deletes expired files, so the description and help now say that directly. The test pinning the exact wording is gone too, since it only asserted the strings back at themselves. 966f8a3. |
| protected function configure(): void { | ||
| parent::configure(); | ||
| $this | ||
| ->setName('trashbin:expire') | ||
| ->setDescription('Expires the users trashbin') | ||
| ->setDescription('Delete expired files from the trashbin') | ||
| ->setHelp('Deletes expired files from the trashbin according to the configured retention and space policy. This does not disable the trashbin or unconditionally empty it.') |
There was a problem hiding this comment.
| ->setHelp('Deletes expired files from the trashbin according to the configured retention and space policy. This does not disable the trashbin or unconditionally empty it.') | |
| ->setHelp('Deletes expired files from the trashbin according to the configured retention and space policy.') |
The help should not detail what the command does not do, that is confusing.
It does not do a lot of other stuff as well.
There was a problem hiding this comment.
Applied in 38e544a - the help string is now exactly your suggestion, with the "does not" clause dropped. Flagging it since the thread went outdated rather than resolved.
Per @come-nc: the help should describe what the command does, not enumerate what it does not do.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
3 participants
Summary
Update the Symfony command metadata in
ExpireTrash::configure()so both the command description and optionaluser_idargument explain that the operation processes deleted files according to the configured trashbin retention and space policy. Make the wording explicit that supplying user IDs limits processing to those users, while omitting them processes all users; avoid implying that the trashbin feature is disabled or that every deleted file is forcibly removed. Preserve the existing execution path, retention behavior, all-user default, and app command registration.The
trashbin:expirecommand currently says that it “expires” a user's trashbin without explaining what expiration does. This leaves administrators unsure whether the command disables the trashbin, empties it unconditionally, or applies the configured retention and space policy. The production path callsTrashbin::expire(), which removes entries eligible undertrashbin_retention_obligationand, where the policy permits, removes oldest entries to bring the trashbin back within its size limit. A recent server change clarified the special disabled-policy runtime output, but the command anduser_idhelp text remain ambiguous; broader documentation is separately tracked innextcloud/documentation#15421.Fixes #45418
TODO
Checklist
Not verified: this needs a person on the named hardware or environment.
3. to review, feature component)Not run: no test command resolved in this workspace, so nothing was executed to pass.
stable32)Not run: no test command resolved in this workspace, so nothing was executed to pass.
AI (if applicable)
AI was used for assistance.