fix: confirmation is broken

danstis commented 7 months ago

User description

Fixes #206, #203

Type

bug_fix

Description

Enhanced the confirmation logic in rmstale.go to skip the confirmation prompt when either dryRun mode is enabled or confirmation is already provided. This improvement addresses issues where automated processes or specific scenarios required bypassing the manual confirmation step, thereby streamlining operations and avoiding unnecessary interruptions.

Changes walkthrough

Relevant files

Enhancement

rmstale.go

Enhance Confirmation Logic for File Removal

rmstale.go

Added a conditional check to only prompt for confirmation if not in
dryRun mode and confirmation is not already provided.

Ensures operation proceeds without unnecessary confirmation prompt in
automated or dry run scenarios.

+5/-3

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

sophie-syntax[bot] commented 7 months ago

PR Description updated to latest commit (https://github.com/danstis/rmstale/commit/8beb4d91db3f196c11dee786e6fb5810bd1bf324)

sophie-syntax[bot] commented 7 months ago

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to a specific logic block within a single file, affecting the confirmation logic. The modification is straightforward and involves adding a conditional check to bypass the confirmation prompt under certain conditions. The logic seems clear, and the amount of changed code is minimal.
🧪 Relevant tests	No
🔍 Possible issues	Possible Logic Issue: The new condition `if !confirm && !dryRun` might inadvertently skip the confirmation prompt in scenarios where `confirm` is false, and `dryRun` is also false, which could lead to unintended deletions if the logic was intended to require explicit confirmation in non-dry run scenarios. It's important to ensure this logic aligns with the intended behavior of the application.
🔒 Security concerns	No

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.

sophie-syntax[bot] commented 7 months ago

PR Code Suggestions

Category Suggestions

Category	Suggestions
Maintainability	Merge nested `if` statements into a single `if` statement. ___ Consider merging the nested `if` statements into a single `if` statement for better readability and to reduce the nesting level. This can be achieved by combining the conditions using logical operators. [rmstale.go [81-84]](https://github.com/danstis/rmstale/pull/207/files#diff-8d2045f56d565d537deafa629d12ea2b52f0701a7366f155b4b1038745e58e4eR81-R84) ```diff -if !confirm && !dryRun { - if !prompt("WARNING: Will remove files and folders recursively below '%v'%s older than %v days.", filepath.FromSlash(folder), extMsg, age) { - logger.Warning("Operation not confirmed, exiting.") - return - } +if !confirm && !dryRun && !prompt("WARNING: Will remove files and folders recursively below '%v'%s older than %v days.", filepath.FromSlash(folder), extMsg, age) { + logger.Warning("Operation not confirmed, exiting.") + return } ```

Maintainability

Merge nested if statements into a single if statement.

___ **Consider merging the nested if statements into a single if statement for better
readability and to reduce the nesting level. This can be achieved by combining the
conditions using logical operators.** [rmstale.go [81-84]](https://github.com/danstis/rmstale/pull/207/files#diff-8d2045f56d565d537deafa629d12ea2b52f0701a7366f155b4b1038745e58e4eR81-R84) ```diff -if !confirm && !dryRun { - if !prompt("WARNING: Will remove files and folders recursively below '%v'%s older than %v days.", filepath.FromSlash(folder), extMsg, age) { - logger.Warning("Operation not confirmed, exiting.") - return - } +if !confirm && !dryRun && !prompt("WARNING: Will remove files and folders recursively below '%v'%s older than %v days.", filepath.FromSlash(folder), extMsg, age) { + logger.Warning("Operation not confirmed, exiting.") + return } ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on a PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L78) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ``` See the improve [usage page](https://pr-agent-docs.codium.ai/tools/improve/) for a comprehensive guide on using this tool.

danstis commented 7 months ago

/similar_issue