feat(bin): x-asdf-cleanup #29
Conversation
|
1 similar comment
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
📝 WalkthroughWalkthroughThe pull request introduces a new Bash script Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as x-asdf-cleanup
participant ASDF as ASDF Version Manager
participant VersionFiles as .tool-versions Files
User->>Script: Run with options
Script->>VersionFiles: Scan for defined versions
Script->>ASDF: Retrieve installed versions
Script->>Script: Compare and identify unused versions
Script->>User: Prompt for uninstallation
User-->>Script: Confirm
Script->>ASDF: Uninstall unused versions
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
ivuorinen
force-pushed
the
feat/x-asdf-cleanup
branch
from
0f3e8bb to
00bb730
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
local/bin/x-asdf-cleanup (4)
17-17: Consider re-enabling 'set -euo pipefail' for robustness.
Having this setting enabled can help catch errors early and avoid unexpected behavior, especially in shell scripts meant for system cleanup and management. Currently, it's commented out, which may lead to silent failures.-# set -euo pipefail +set -euo pipefail
28-34: Add error handling for plugin installation failure.
If the command to install fd with asdf fails or is not permitted, the script will exit with a missing dependency. Consider adding error-handling logic or guidance to the user for manual installation.
211-244: Enhance error handling for 'asdf list' parsing.
If 'asdf list' fails or its output format changes, the loop parsing the tool versions might break silently. Consider checking the exit status of 'asdf list' or validating the contents before processing.
330-357: Consider breaking the 'main' function into smaller, focused functions.
The logic within 'main' is somewhat lengthy (parsing, directory checks, reading versions, displaying, uninstalling). Splitting it into smaller functions can improve readability, maintainability, and testability.config/nvim/lua/plugins/neogen.lua (1)
6-6: Consider pinning to a specific versionUsing
version = '*'could lead to unexpected breaking changes when the plugin updates. Consider pinning to a specific version for better stability.- version = '*', + version = '^0.10.0', -- Replace with the latest stable versionconfig/nvim/lua/plugins/lsp-saga.lua (1)
7-10: Mark dependencies as optional in configurationThe dependencies are marked as optional in comments, but this should be reflected in the configuration itself using the
optional = truefield.dependencies = { - 'nvim-treesitter/nvim-treesitter', -- optional - 'nvim-tree/nvim-web-devicons', -- optional + { 'nvim-treesitter/nvim-treesitter', optional = true }, + { 'nvim-tree/nvim-web-devicons', optional = true }, },config/nvim/lua/plugins/flash.lua (1)
1-37: Add configuration documentationConsider adding comments explaining the purpose of each key mapping group for better maintainability.
return { 'folke/flash.nvim', event = 'VeryLazy', opts = {}, keys = { + -- Basic jump functionality { 'zk', mode = { 'n', 'x', 'o' }, function() require('flash').jump() end, desc = 'Flash', }, + -- Treesitter-based navigation { 'Zk', mode = { 'n', 'x', 'o' }, function() require('flash').treesitter() end, desc = 'Flash Treesitter', },config/nvim/lua/plugins/harpoon.lua (2)
16-32: Optimize telescope configuration for better performance.The
toggle_telescopefunction creates a new table on each call. Consider memoizing the picker configuration for better performance.-local function toggle_telescope(harpoon_files) +local picker_conf = { + prompt_title = 'Harpoon', + previewer = conf.file_previewer {}, + sorter = conf.generic_sorter {}, +} + +local function toggle_telescope(harpoon_files) local file_paths = {} for _, item in ipairs(harpoon_files.items) do table.insert(file_paths, item.value) end require('telescope.pickers') .new({}, { - prompt_title = 'Harpoon', + prompt_title = picker_conf.prompt_title, finder = require('telescope.finders').new_table { results = file_paths, }, - previewer = conf.file_previewer {}, - sorter = conf.generic_sorter {}, + previewer = picker_conf.previewer, + sorter = picker_conf.sorter, }) :find() end
34-45: Consider adding more descriptive key mapping descriptions.The current descriptions are good but could be more specific about what the commands do.
-{ desc = 'Open harpoon window with telescope' } +{ desc = 'Open Harpoon file list in Telescope' } -{ desc = 'Open Harpoon Quick menu' } +{ desc = 'Toggle Harpoon quick menu overlay' }config/nvim/lua/plugins/conform.lua (2)
15-22: Remove redundant comments and consolidate default formatter configuration.There are duplicate comments about setting the default formatter, and the commented-out
formatter_optsadds noise.default_formatter = 'injected', - -- Set the default formatter for all filetypes default_formatter_opts = { lsp_format = 'fallback', - -- Set the default formatter for all filetypes formatter = 'injected', - -- Set the default formatter for all filetypes - -- formatter_opts = {}, },
46-69: Refactor format-on-save function for better maintainability.The function contains multiple conditions that could be extracted into separate functions for better readability and maintainability.
+local function should_ignore_filetype(bufnr) + local ignore_filetypes = { 'c', 'cpp', 'sql', 'java' } + return vim.tbl_contains(ignore_filetypes, vim.bo[bufnr].filetype) +end + +local function is_formatting_disabled(bufnr) + return vim.g.disable_autoformat or vim.b[bufnr].disable_autoformat +end + +local function is_ignored_path(bufname) + local ignored_patterns = { + '/node_modules/', + '/vendor/', + '/dist/', + '/build/', + } + for _, pattern in ipairs(ignored_patterns) do + if bufname:match(pattern) then + return true + end + end + return false +end + format_on_save = function(bufnr) - -- Disable autoformat on certain filetypes - local ignore_filetypes = { - 'c', - 'cpp', - 'sql', - 'java', - } - if vim.tbl_contains(ignore_filetypes, vim.bo[bufnr].filetype) then + if should_ignore_filetype(bufnr) then return end - -- Disable with a global or buffer-local variable - if vim.g.disable_autoformat or vim.b[bufnr].disable_autoformat then + if is_formatting_disabled(bufnr) then return end - -- Disable autoformat for files in a certain path local bufname = vim.api.nvim_buf_get_name(bufnr) - if bufname:match '/node_modules/' then return end - if bufname:match '/vendor/' then return end - if bufname:match '/dist/' then return end - if bufname:match '/build/' then return end + if is_ignored_path(bufname) then + return + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
config/exports(1 hunks)config/nvim/lua/plugins/comment.lua(1 hunks)config/nvim/lua/plugins/conform.lua(1 hunks)config/nvim/lua/plugins/flash.lua(1 hunks)config/nvim/lua/plugins/harpoon.lua(1 hunks)config/nvim/lua/plugins/lsp-saga.lua(1 hunks)config/nvim/lua/plugins/neogen.lua(1 hunks)config/nvim/lua/plugins/refactoring.lua(1 hunks)config/nvim/lua/plugins/surround.lua(1 hunks)config/nvim/lua/plugins/todo-comments.lua(1 hunks)config/nvim/lua/plugins/treesj.lua(1 hunks)config/nvim/lua/plugins/ufo.lua(1 hunks)config/nvim/lua/plugins/vim-sleuth.lua(1 hunks)local/bin/x-asdf-cleanup(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- config/nvim/lua/plugins/vim-sleuth.lua
- config/nvim/lua/plugins/refactoring.lua
- config/nvim/lua/plugins/todo-comments.lua
- config/exports
🔇 Additional comments (10)
config/nvim/lua/plugins/ufo.lua (5)
1-2: Great descriptive header and reference link.
It's helpful that you included a short comment and the GitHub link for context and documentation.
15-37: Clear configuration for 'statuscol.nvim'.
The presence of well-documented segments with click-handlers provides a good user experience for fold functions, diagnostic signs, and line numbers. Nicely done!
41-53: LSP capabilities integration is well-structured.
Enabling foldingRange with dynamic registration ensures that your fold provider adapts to each LSP. Good approach.
55-71: Neat UFO plugin setup.
The configurations for open/close fold highlights, border, and scrolling mappings look correct. This provides a more intuitive folding experience.
76-119: Custom fold text handler is elegantly implemented.
The thorough inline documentation for folding lines is appreciated. The logic for displaying the truncated text and suffix ensures a better reading experience in folded lines.
config/nvim/lua/plugins/surround.lua (1)
Line range hint 3-9: Straightforward plugin addition.
This configuration is succinct and properly references the plugin repository. Loading on 'VeryLazy' helps optimize startup.
config/nvim/lua/plugins/comment.lua (1)
1-9: Solid addition of Comment.nvim integration.
The usage notes and the event triggers for 'BufRead' and 'BufNewFile' will ensure convenient plugin loading for commenting functionality.
config/nvim/lua/plugins/treesj.lua (1)
3-9: Nice plugin for splitting/joining code blocks.
Disabling the default keymaps offers flexibility for custom mappings. The chosen approach is straightforward and clean.
config/nvim/lua/plugins/flash.lua (1)
7-11: Verify key mapping conflicts
The zk mapping might conflict with popular note-taking plugins like zk-nvim. Consider documenting this potential conflict or using a different mapping.
config/nvim/lua/plugins/harpoon.lua (1)
4-9: Consider pinning the dependency versions for stability.
The plugin configuration uses the harpoon2 branch without specifying a specific commit or version. This could lead to unexpected behavior if the branch is updated.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
local/bin/x-asdf-cleanup (1)
366-368: Use local declarations for arrays in main functionThe associative arrays should be declared as local to prevent global namespace pollution:
- declare -A defined_versions - declare -A installed_versions - declare -A keep_version + local -A defined_versions + local -A installed_versions + local -A keep_version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/exports(1 hunks)local/bin/x-asdf-cleanup(1 hunks)
🔇 Additional comments (2)
local/bin/x-asdf-cleanup (2)
338-340: Add error handling for asdf uninstall command failures
The current implementation doesn't check if the uninstall command succeeds.
177-184:
Security: Avoid using eval with command construction
The use of eval with $fd_command is potentially dangerous if BASE_DIR contains malicious characters. Consider using an array to build and execute the command:
- local fd_command="fd --base-directory $BASE_DIR --glob '$FILE' --hidden"
- for pattern in "${EXCLUDE_PATTERNS[@]}"; do
- fd_command="$fd_command --exclude $pattern"
- done
- eval "$fd_command"
+ local -a cmd=(fd --base-directory "$BASE_DIR" --glob "$FILE" --hidden)
+ for pattern in "${EXCLUDE_PATTERNS[@]}"; do
+ cmd+=(--exclude "$pattern")
+ done
+ "${cmd[@]}"Likely invalid or redundant comment.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
local/bin/x-asdf-cleanup (3)
177-184:⚠️ Potential issueSecurity: Avoid using 'eval' for command construction
Using 'eval' can introduce security risks if BASE_DIR or excluded patterns contain malicious characters.
Apply this fix:
find_version_files() { local FILE="$1" - local fd_command="fd --base-directory $BASE_DIR --glob '$FILE' --hidden" + local args=("--base-directory" "$BASE_DIR" "--glob" "$FILE" "--hidden") for pattern in "${EXCLUDE_PATTERNS[@]}"; do - fd_command="$fd_command --exclude $pattern" + args+=("--exclude" "$pattern") done - eval "$fd_command" + fd "${args[@]}" }
189-191:⚠️ Potential issueFix: Helper functions incorrectly echo command name instead of executing it
The helper functions are incorrectly echoing the command name instead of executing it.
Apply this fix to all helper functions:
find_tool_versions_files() { - echo find_version_files ".tool-versions" + find_version_files ".tool-versions" } find_nvmrc_files() { - echo find_version_files ".nvmrc" + find_version_files ".nvmrc" } find_python_version_files() { - echo find_version_files ".python-version" + find_version_files ".python-version" }Also applies to: 196-198, 203-205
319-347:⚠️ Potential issueAdd error handling for
asdf uninstallcommand failuresThe uninstall_versions function doesn't check the exit status of
asdf uninstall.Apply this fix:
if [ "$DRYRUN" = true ]; then echo "(?) Dry run: would uninstall $name $version" else echo "(*) Uninstalling $name: $version" - asdf uninstall $name $version + if ! asdf uninstall "$name" "$version"; then + echo "(!) Failed to uninstall $name: $version" + # Continue with remaining uninstallations + fi fi
🧹 Nitpick comments (1)
local/bin/x-asdf-cleanup (1)
352-381: Add cleanup confirmation before startingWhen not in dry run mode, it would be helpful to ask for confirmation before starting the cleanup process, as it might take time for large codebases.
Apply this enhancement:
main() { parse_arguments "$@" BASEDIR_IS_DIR=$(is_dir "$BASE_DIR") if [ "$BASEDIR_IS_DIR" = "no" ]; then echo "(!) Base directory $BASE_DIR does not exist or is not a directory" exit 1 fi + if [ "$DRYRUN" = false ]; then + read -p "Start cleanup in $BASE_DIR? This might take a while. (y/N): " confirm + if [[ ! "$confirm" =~ ^[Yy]$ ]]; then + echo "Cleanup aborted by user." + exit 0 + fi + fi + files=$(find_tool_versions_files) echo "Found .tool-versions files:" echo "$files" echo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
local/bin/x-asdf-cleanup(1 hunks)
🔇 Additional comments (1)
local/bin/x-asdf-cleanup (1)
1-40: LGTM! Well-structured initialization with proper dependency checks.
The script follows best practices with:
- Comprehensive documentation
- Strict mode enabled
- Thorough dependency checks with helpful error messages
- Proper debug mode implementation
ivuorinen
force-pushed
the
feat/x-asdf-cleanup
branch
from
c6eac51 to
663f4b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
local/bin/x-asdf-cleanup (4)
23-26: Enhance dependency checks with version requirementsThe dependency checks could be improved by verifying minimum required versions:
if ! command -v asdf &> /dev/null; then echo "(!) asdf itself is not installed or not in PATH" exit 1 +else + # Verify minimum asdf version + local asdf_version=$(asdf --version | cut -d' ' -f1) + if ! printf '%s\n%s\n' "0.10.0" "$asdf_version" | sort -V -C; then + echo "(!) asdf version >= 0.10.0 is required" + exit 1 + fi fi if ! command -v fd &> /dev/null; then echo "(!) Required tool fd is not installed or not in PATH" echo "It's used to find .tool-versions files faster" echo "Install it with asdf:" echo "asdf plugin add fd && asdf install fd latest" exit 1 +else + # Verify minimum fd version + local fd_version=$(fd --version | cut -d' ' -f2) + if ! printf '%s\n%s\n' "8.0.0" "$fd_version" | sort -V -C; then + echo "(!) fd version >= 8.0.0 is required" + exit 1 + fi fiAlso applies to: 28-34
108-115: Add error handling to is_dir functionThe function should handle cases where the directory parameter is empty or contains special characters:
is_dir() { local dir="$1" + if [ -z "$dir" ]; then + echo "(!) Empty directory parameter" >&2 + return 1 + fi if [ -d "$dir" ]; then echo "yes" else echo "no" fi }
305-315: Enhance version comparison logicThe current implementation might miss versions that are semantically equivalent but syntactically different (e.g., "1.0.0" vs "1.0"):
determine_versions_to_uninstall() { echo "" echo "Versions to Uninstall:" for key in "${!installed_versions[@]}"; do - if [[ -z ${keep_version[$key]} ]]; then + local name="${key% *}" + local version="${key#* }" + local normalized_version=$(normalize_version "$version") + local normalized_key="$name $normalized_version" + if [[ -z ${keep_version[$normalized_key]} ]]; then echo "$key" uninstall_list+=("$key") fi done echo }Add this helper function at the top of the file:
# Normalize version string for comparison # Example: normalize_version "1.0" returns "1.0.0" normalize_version() { echo "$1" | awk -F. '{ printf("%d.%d.%d\n", $1,$2,$3) }' 2>/dev/null || echo "$1" }
350-379: Improve main function organization and cleanup
- Move array declarations to the beginning
- Add cleanup handling
# Main script execution main() { + # Declare associative arrays at the beginning + declare -A defined_versions + declare -A installed_versions + declare -A keep_version + + # Set up cleanup trap + cleanup() { + unset defined_versions installed_versions keep_version + # Add any other cleanup needed + } + trap cleanup EXIT + parse_arguments "$@" BASEDIR_IS_DIR=$(is_dir "$BASE_DIR") if [ "$BASEDIR_IS_DIR" = "no" ]; then echo "(!) Base directory $BASE_DIR does not exist or is not a directory" exit 1 fi files=$(find_tool_versions_files) echo "Found .tool-versions files:" echo "$files" echo - # Declare associative arrays - declare -A defined_versions - declare -A installed_versions - declare -A keep_version read_defined_versions display_defined_versions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/exports(1 hunks)local/bin/x-asdf-cleanup(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/exports
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
local/bin/x-asdf-cleanup (3)
121-126: Ensure proper cleanup in error handler.The
error_handlerfunction should remove temporary files before calling cleanup to prevent resource leaks.error_handler() { local exit_code=$1 print_error "An error occurred (exit code: $exit_code)" + rm -f "$TMPFILE" cleanup }
179-186: Optimize debug output handling.The
debugfunction writes to stderr unconditionally, which could impact performance when debug mode is off.debug() { if [[ $DEBUG == true ]]; then local timestamp timestamp=$(date '+%Y-%m-%d %H:%M:%S') - echo "[$timestamp][DEBUG] $*" >&2 + printf '[%s][DEBUG] %s\n' "$timestamp" "$*" >&2 fi }
1199-1276: Improve signal handling in main function.The main function should handle additional signals and ensure proper cleanup.
# Main execution main() { + # Set up signal handlers + trap 'cleanup' SIGTERM SIGINT SIGQUIT + check_dependencies initialize_directories load_config # ... rest of the function ... }local/bin/x-asdf-cleanup.md (3)
73-77: Enhance performance settings documentationConsider adding explanations for:
- How
MAX_PARALLEL_JOBSaffects system resources and cleanup speed- What data is cached and how
USE_CACHEandCACHE_MAX_AGEimpact performance
107-111: Fix grammar and enhance example with output
- Fix the grammar in the heading:
-Dry run to see what would be uninstalled: +Dry-run to see what would be uninstalled:
- Consider adding example output to show users what to expect:
x-asdf-cleanup --dry-run # Example output: # Found unused versions: # nodejs 14.17.0 # python 3.8.5 # These versions would be backed up and removed.🧰 Tools
🪛 LanguageTool
[grammar] ~107-~107: The verb form doesn’t seem to match ‘Dry’. If it is an imperative, consider using a comma.
Context: ...ormation and exit### Examples Dry run to see what would be uninstalled:b...(NNP_VB_TO)
142-148: Fix grammar in contributing guidelinesStandardize the format of the list items:
3. Follow the coding standards: -- Use shellcheck for bash script linting -- Add comments for complex logic -- Update documentation as needed -- Add error handling for new functions +- Use shellcheck for bash script linting +- Add comments for complex logic +- Update documentation as needed +- Add error handling for new functions🧰 Tools
🪛 LanguageTool
[grammar] ~147-~147: Did you mean “to Add”?
Context: ...ogic - Update documentation as needed - Add error handling for new functions 4. Te...(MISSING_TO_BEFORE_A_VERB)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.markdownlint.json(1 hunks).pre-commit-config.yaml(1 hunks)local/bin/x-asdf-cleanup(1 hunks)local/bin/x-asdf-cleanup.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
local/bin/x-asdf-cleanup.md
[grammar] ~107-~107: The verb form doesn’t seem to match ‘Dry’. If it is an imperative, consider using a comma.
Context: ...ormation and exit ### Examples Dry run to see what would be uninstalled: b...
(NNP_VB_TO)
[grammar] ~147-~147: Did you mean “to Add”?
Context: ...ogic - Update documentation as needed - Add error handling for new functions 4. Te...
(MISSING_TO_BEFORE_A_VERB)
🔇 Additional comments (4)
.markdownlint.json (1)
16-16: LGTM! Configuration change is appropriate.
The addition of "ol-prefix": false aligns well with the documentation needs and maintains consistency with existing markdown configuration.
.pre-commit-config.yaml (1)
28-28: LGTM! Configuration path updated correctly.
The markdownlint configuration path has been properly updated to reference the JSON configuration file, maintaining consistency with the previous change.
local/bin/x-asdf-cleanup (1)
16-16: 🛠️ Refactor suggestion
Enable full strict mode for better error handling.
The script only uses set -u but would benefit from full strict mode to catch errors early and prevent subtle bugs.
-#set -euo pipefail
+set -euo pipefailLikely invalid or redundant comment.
local/bin/x-asdf-cleanup.md (1)
36-55: LGTM! Well-organized directory structure
The directory structure follows the XDG Base Directory Specification correctly, with clear separation of binaries, state data, configuration, and cache.
| --base-dir=*) | ||
| BASE_DIR="${arg#*=}" | ||
| ;; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate base directory path in argument parsing.
The base directory path should be validated when it's provided as an argument.
--base-dir=*)
BASE_DIR="${arg#*=}"
+ if [[ ! -d $BASE_DIR ]]; then
+ handle_error 1 "Invalid base directory: $BASE_DIR"
+ fi
+ if [[ ! -r $BASE_DIR ]]; then
+ handle_error 1 "Base directory not readable: $BASE_DIR"
+ fi
;;📝 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.
| --base-dir=*) | |
| BASE_DIR="${arg#*=}" | |
| ;; | |
| --base-dir=*) | |
| BASE_DIR="${arg#*=}" | |
| if [[ ! -d $BASE_DIR ]]; then | |
| handle_error 1 "Invalid base directory: $BASE_DIR" | |
| fi | |
| if [[ ! -r $BASE_DIR ]]; then | |
| handle_error 1 "Base directory not readable: $BASE_DIR" | |
| fi | |
| ;; |
| process_tool_versions_file() | ||
| { | ||
| local file="$1" | ||
| if [[ ! -f $file ]]; then | ||
| debug "File not found: $file" | ||
| return 1 | ||
| fi | ||
| # Process file content and output "tool version" pairs | ||
| # Ignore comments and empty lines, require at least tool and version | ||
| awk '!/#/ && NF >= 2 {print $1, $2}' "$file" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file read operations.
The process_tool_versions_file function should handle file read errors properly.
process_tool_versions_file()
{
local file="$1"
if [[ ! -f $file ]]; then
debug "File not found: $file"
return 1
fi
+ if [[ ! -r $file ]]; then
+ debug "File not readable: $file"
+ return 1
+ fi
# Process file content and output "tool version" pairs
# Ignore comments and empty lines, require at least tool and version
awk '!/#/ && NF >= 2 {print $1, $2}' "$file"
}📝 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.
| process_tool_versions_file() | |
| { | |
| local file="$1" | |
| if [[ ! -f $file ]]; then | |
| debug "File not found: $file" | |
| return 1 | |
| fi | |
| # Process file content and output "tool version" pairs | |
| # Ignore comments and empty lines, require at least tool and version | |
| awk '!/#/ && NF >= 2 {print $1, $2}' "$file" | |
| } | |
| process_tool_versions_file() | |
| { | |
| local file="$1" | |
| if [[ ! -f $file ]]; then | |
| debug "File not found: $file" | |
| return 1 | |
| fi | |
| if [[ ! -r $file ]]; then | |
| debug "File not readable: $file" | |
| return 1 | |
| fi | |
| # Process file content and output "tool version" pairs | |
| # Ignore comments and empty lines, require at least tool and version | |
| awk '!/#/ && NF >= 2 {print $1, $2}' "$file" | |
| } |
| 4. Run tests: | ||
|
|
||
| ```bash | ||
| shellcheck x-asdf-cleanup | ||
| shfmt -d x-asdf-cleanup | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance testing framework
The current testing approach only covers linting. Consider adding:
- Unit tests for core functionality
- Integration tests for version management
- Test cases for error scenarios
Would you like me to help create a test framework with example test cases?
There was a problem hiding this comment.
Please create tests, and keep all tests in a single file, like x-asdf-cleanup.tests.sh or what ever the extension should be.
There was a problem hiding this comment.
@ivuorinen Here’s an example Bash test file named x-asdf-cleanup.tests.sh that places all tests in one file, as requested. This script demonstrates a simple approach to testing, including checking various command-line options, verifying expected outputs, and handling error scenarios. You can expand upon these examples to add more specific integration or unit tests as needed:
#!/usr/bin/env bash
#
# x-asdf-cleanup.tests.sh - Basic test suite for x-asdf-cleanup
# Path to the script under test (adjust if placed elsewhere)
SCRIPT_PATH="./x-asdf-cleanup"
# Helper function to print results in a uniform manner
print_result() {
local test_name="$1"
local result="$2"
if [[ "$result" -eq 0 ]]; then
echo "[PASS] $test_name"
else
echo "[FAIL] $test_name"
exit 1
fi
}
# Test: Script should display help information and exit with code 0
test_help_option() {
local output
output="$($SCRIPT_PATH --help 2>&1)"
local exit_code=$?
# Check exit code
if [[ $exit_code -ne 0 ]]; then
echo "Expected exit code 0 for --help, got $exit_code"
return 1
fi
# Check for expected content in help output
grep -q "Usage" <<< "$output" || {
echo "'Usage' keyword missing from help output"
return 1
}
return 0
}
# Test: Dry run should not remove any versions
test_dry_run_option() {
# If feasible, set up a known scenario (e.g., mock some .tool-versions)
# For demonstration, we just check that the command runs successfully.
local output
output="$($SCRIPT_PATH --dry-run 2>&1)"
local exit_code=$?
# Check exit code
if [[ $exit_code -ne 0 ]]; then
echo "Expected exit code 0 for --dry-run, got $exit_code | output: $output"
return 1
fi
# Optionally, check if the output mentions 'dry run' or no changes made
grep -iq "dry run" <<< "$output" || {
echo "Expected 'dry run' mention in output, got: $output"
return 1
}
return 0
}
# Test: Invalid option should fail
test_invalid_option() {
local output
output="$($SCRIPT_PATH --not-real 2>&1)"
local exit_code=$?
# We expect a non-zero exit code
if [[ $exit_code -eq 0 ]]; then
echo "Expected non-zero exit code for invalid option"
return 1
fi
return 0
}
main() {
echo "Running tests for x-asdf-cleanup..."
test_help_option
print_result "test_help_option" $?
test_dry_run_option
print_result "test_dry_run_option" $?
test_invalid_option
print_result "test_invalid_option" $?
# Additional tests can be added below:
# - Checking if versions are actually uninstalled
# - Testing backups/restore functionality
# - Verifying performance or parallel run configurations
echo "All tests completed."
}
mainYou can run these tests by making the file executable:
chmod +x x-asdf-cleanup.tests.sh
./x-asdf-cleanup.tests.sh✏️ Learnings added
Learnt from: ivuorinen
PR: ivuorinen/dotfiles#29
File: local/bin/x-asdf-cleanup.md:197-202
Timestamp: 2024-12-30T13:11:43.500Z
Learning: The user wants all tests for x-asdf-cleanup in a single file named x-asdf-cleanup.tests.sh or a similar extension.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
local/bin/x-asdf-cleanup.md (3)
5-16: Add troubleshooting and error messages sections.Consider adding the following sections to enhance user experience:
- Common issues and their solutions
- List of error messages and their meanings
- Known limitations
61-80: Enhance configuration examples with real-world scenarios.Add more comprehensive configuration examples:
- Project-specific configurations
- Team-wide configurations
- CI/CD environment configurations
- Performance tuning examples
142-148: Expand contributing guidelines for better consistency.Add more detailed contributing guidelines:
- Code style guide with examples
- Commit message format
- Pull request template
- Review process expectations
- Release process
🧰 Tools
🪛 LanguageTool
[grammar] ~147-~147: Did you mean “to Add”?
Context: ...ogic - Update documentation as needed - Add error handling for new functions 4. Te...(MISSING_TO_BEFORE_A_VERB)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
local/bin/x-asdf-cleanup(1 hunks)local/bin/x-asdf-cleanup.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
local/bin/x-asdf-cleanup.md
[grammar] ~107-~107: The verb form doesn’t seem to match ‘Dry’. If it is an imperative, consider using a comma.
Context: ...ormation and exit ### Examples Dry run to see what would be uninstalled: b...
(NNP_VB_TO)
[grammar] ~147-~147: Did you mean “to Add”?
Context: ...ogic - Update documentation as needed - Add error handling for new functions 4. Te...
(MISSING_TO_BEFORE_A_VERB)
🔇 Additional comments (4)
local/bin/x-asdf-cleanup.md (2)
3-4: Add security considerations section.
The tool performs system-wide changes. Consider adding a security section that covers:
- Permission requirements and implications
- Backup and restore safety measures
- Risk mitigation for automatic version removal
197-202: Enhance testing framework documentation.
The current testing approach only covers linting. Consider documenting:
- Unit testing strategy and examples
- Integration testing guidelines
- Error scenario testing procedures
local/bin/x-asdf-cleanup (2)
16-16: 🛠️ Refactor suggestion
Enable strict mode for better error handling.
Enable full strict mode to catch errors early and prevent subtle bugs.
-set -u
+set -euo pipefailLikely invalid or redundant comment.
1219-1230: 🛠️ Refactor suggestion
Enhance main function validation.
Add comprehensive validation in the main function:
- Validate base directory permissions
- Check available system resources for parallel jobs
- Verify write permissions for output directories
main() {
check_dependencies
initialize_directories
load_config
parse_arguments "$@"
initialize_cache
+ # Validate base directory
if [[ $(is_dir "$BASE_DIR") == "no" ]]; then
handle_error 1 "Base directory $BASE_DIR does not exist or is not a directory"
fi
+ if [[ ! -r "$BASE_DIR" ]]; then
+ handle_error 1 "Base directory $BASE_DIR is not readable"
+ fi
+
+ # Validate parallel job count
+ local max_jobs
+ max_jobs=$(nproc)
+ if ((MAX_PARALLEL_JOBS > max_jobs)); then
+ print_warning "Reducing parallel jobs from $MAX_PARALLEL_JOBS to $max_jobs"
+ MAX_PARALLEL_JOBS=$max_jobs
+ fiLikely invalid or redundant comment.
| trap 'cleanup' INT TERM | ||
| trap 'error_handler $?' ERR | ||
| trap 'error_occurred $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR | ||
| trap 'rm -f "$TMPFILE"' EXIT |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate error handling traps.
The error handling traps have overlapping functionality. Consider consolidating them into a single, comprehensive error handler.
-trap 'cleanup' INT TERM
-trap 'error_handler $?' ERR
-trap 'error_occurred $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR
+trap 'error_handler $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR
+trap 'cleanup' INT TERM HUP
trap 'rm -f "$TMPFILE"' EXIT📝 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.
| trap 'cleanup' INT TERM | |
| trap 'error_handler $?' ERR | |
| trap 'error_occurred $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR | |
| trap 'rm -f "$TMPFILE"' EXIT | |
| trap 'error_handler $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR | |
| trap 'cleanup' INT TERM HUP | |
| trap 'rm -f "$TMPFILE"' EXIT |
| version_compare() | ||
| { | ||
| local IFS=. | ||
| local i ver1=($1) ver2=($2) | ||
|
|
||
| debug "Comparing versions: $1 vs $2" | ||
|
|
||
| for ((i = 0; i < ${#ver1[@]} || i < ${#ver2[@]}; i++)); do | ||
| local v1=${ver1[i]:-0} v2=${ver2[i]:-0} | ||
| debug "Comparing components: $v1 vs $v2" | ||
|
|
||
| ((v1 > v2)) && { | ||
| debug "First version is greater" | ||
| return 1 | ||
| } | ||
| ((v1 < v2)) && { | ||
| debug "First version is less" | ||
| return 2 | ||
| } | ||
| done | ||
|
|
||
| debug "Versions are equal" | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance version comparison function.
The version comparison function needs improvements:
- Handle pre-release versions (e.g., alpha, beta)
- Support build metadata
- Validate version format before comparison
version_compare() {
+ # Validate version format
+ local version_regex='^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$'
+ if ! [[ $1 =~ $version_regex && $2 =~ $version_regex ]]; then
+ debug "Invalid version format: $1 or $2"
+ return 255
+ fi
+
+ # Extract pre-release and build metadata
+ local v1_core="${1%%-*}" v2_core="${2%%-*}"
+ local v1_pre="${1#*-}" v2_pre="${2#*-}"
+ [[ "$v1_pre" == "$1" ]] && v1_pre="" || v1_pre="${v1_pre%+*}"
+ [[ "$v2_pre" == "$2" ]] && v2_pre="" || v2_pre="${v2_pre%+*}"
+
local IFS=.
- local i ver1=($1) ver2=($2)
+ local i ver1=($v1_core) ver2=($v2_core)
# ... rest of the functionCommittable suggestion skipped: line range outside the PR's diff.
added snippets!
ivuorinen
force-pushed
the
feat/x-asdf-cleanup
branch
from
9a6b255 to
c8d8b9a
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
local/bin/x-asdf-cleanup.md (1)
61-80: Add configuration validation and impact descriptions.Consider enhancing the configuration example with:
- Valid value ranges for each setting
- Impact descriptions
- Default values
```bash # Base directory for searching version files +# Default: $HOME +# Impact: Determines the root directory for version file scanning BASE_DIR="$HOME/projects" # Additional directories to exclude +# Default: [] +# Impact: Reduces scan time and prevents unnecessary searches EXCLUDE_PATTERNS+=( "node_modules" "vendor" "dist" ".venv" ) # Performance settings +# Valid range: 1-16 +# Impact: Higher values increase CPU usage but reduce execution time MAX_PARALLEL_JOBS=8 + +# Valid values: true/false +# Impact: Caching improves performance for repeated runs USE_CACHE=true + +# Valid range: 60-86400 (1 minute to 24 hours) +# Impact: Lower values ensure more up-to-date results but increase scan time CACHE_MAX_AGE=3600 # 1 hour in seconds # Output settings +# Valid values: true/false +# Impact: Enables detailed progress information VERBOSE=falselocal/bin/x-asdf-cleanup (1)
22-24: Validate cache configuration values.Cache settings should be validated to ensure they are within reasonable bounds.
# Additional configuration +readonly MIN_CACHE_AGE=60 # 1 minute +readonly MAX_CACHE_AGE=86400 # 24 hours USE_CACHE=true -CACHE_MAX_AGE=3600 # 1 hour in seconds +CACHE_MAX_AGE=3600 # 1 hour in seconds + +if ((CACHE_MAX_AGE < MIN_CACHE_AGE || CACHE_MAX_AGE > MAX_CACHE_AGE)); then + echo "Error: CACHE_MAX_AGE must be between $MIN_CACHE_AGE and $MAX_CACHE_AGE seconds" + exit 1 +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.markdownlint.json(1 hunks).pre-commit-config.yaml(1 hunks)local/bin/x-asdf-cleanup(1 hunks)local/bin/x-asdf-cleanup.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- .markdownlint.json
🧰 Additional context used
📓 Learnings (1)
local/bin/x-asdf-cleanup.md (1)
Learnt from: ivuorinen
PR: ivuorinen/dotfiles#29
File: local/bin/x-asdf-cleanup.md:197-202
Timestamp: 2024-12-30T13:11:43.623Z
Learning: The user wants all tests for x-asdf-cleanup in a single file named x-asdf-cleanup.tests.sh or a similar extension.
🪛 LanguageTool
local/bin/x-asdf-cleanup.md
[grammar] ~107-~107: The verb form doesn’t seem to match ‘Dry’. If it is an imperative, consider using a comma.
Context: ...ormation and exit ### Examples Dry run to see what would be uninstalled: b...
(NNP_VB_TO)
[grammar] ~147-~147: Did you mean “to Add”?
Context: ...ogic - Update documentation as needed - Add error handling for new functions 4. Te...
(MISSING_TO_BEFORE_A_VERB)
🔇 Additional comments (6)
local/bin/x-asdf-cleanup (6)
913-954: 🛠️ Refactor suggestion
Add error handling for asdf command failures.
The version reading function should handle asdf command failures gracefully.
read_installed_versions() {
local current_tool=""
local line
debug "Reading installed versions from asdf"
+ # Capture asdf output and check for errors
+ local asdf_output
+ if ! asdf_output=$(asdf list 2>&1); then
+ print_error "Failed to read installed versions: $asdf_output"
+ log_to_file "ERROR" "Failed to read installed versions: $asdf_output"
+ return 1
+ fi
+
- while IFS= read -r line; do
+ while IFS= read -r line || [[ -n $line ]]; do
# ... rest of the function
- done < <(asdf list)
+ done <<< "$asdf_output"
}Likely invalid or redundant comment.
16-16: 🛠️ Refactor suggestion
Enable strict mode for better error handling.
The script only sets -u but should enable full strict mode for better error handling.
-set -u
+set -euo pipefailLikely invalid or redundant comment.
42-45: 🛠️ Refactor suggestion
Consolidate error handling traps.
Multiple overlapping error traps can lead to unpredictable behavior.
-trap 'cleanup' INT TERM
-trap 'error_handler $?' ERR
-trap 'error_occurred $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR
+trap 'error_handler $? $LINENO $BASH_LINENO "$BASH_COMMAND" $(printf "::%s" ${FUNCNAME[@]:-})' ERR
+trap 'cleanup' INT TERM HUPLikely invalid or redundant comment.
849-880: 🛠️ Refactor suggestion
Add backup directory permission checks and backup integrity verification.
The backup function should validate directory permissions and verify backup integrity.
create_backup() {
local tool=$1
local version=$2
local backup_path="$BACKUP_DIR/${tool}_${version}_$(date +%Y%m%d_%H%M%S)"
local tool_path
tool_path=$(asdf where "$tool" "$version")
+ # Check backup directory permissions
+ if [[ ! -w $BACKUP_DIR ]]; then
+ debug "Backup directory not writable: $BACKUP_DIR"
+ return 1
+ fi
+
if [[ -d $tool_path ]]; then
debug "Creating backup of $tool $version"
log_to_file "INFO" "Creating backup of $tool $version"
if tar czf "$backup_path.tar.gz" -C "$(dirname "$tool_path")" "$(basename "$tool_path")"; then
+ # Verify backup integrity
+ if ! tar tzf "$backup_path.tar.gz" >/dev/null 2>&1; then
+ debug "Backup verification failed"
+ rm -f "$backup_path.tar.gz"
+ return 1
+ fi
echo "$tool $version" > "$LAST_OP_FILE"
return 0
fi
fi
return 1
}Likely invalid or redundant comment.
1024-1026: 🛠️ Refactor suggestion
Validate base directory path in argument parsing.
The base directory path should be validated when it's provided as an argument.
--base-dir=*)
BASE_DIR="${arg#*=}"
+ if [[ ! -d $BASE_DIR ]]; then
+ handle_error 1 "Invalid base directory: $BASE_DIR"
+ fi
+ if [[ ! -r $BASE_DIR ]]; then
+ handle_error 1 "Base directory not readable: $BASE_DIR"
+ fi
;;Likely invalid or redundant comment.
350-373: 🛠️ Refactor suggestion
Enhance version comparison function.
The version comparison function needs improvements:
- Handle pre-release versions (e.g., alpha, beta)
- Support build metadata
- Validate version format before comparison
version_compare() {
+ # Validate version format
+ local version_regex='^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$'
+ if ! [[ $1 =~ $version_regex && $2 =~ $version_regex ]]; then
+ debug "Invalid version format: $1 or $2"
+ return 255
+ fi
+
+ # Extract pre-release and build metadata
+ local v1_core="${1%%-*}" v2_core="${2%%-*}"
+ local v1_pre="${1#*-}" v2_pre="${2#*-}"
+ [[ "$v1_pre" == "$1" ]] && v1_pre="" || v1_pre="${v1_pre%+*}"
+ [[ "$v2_pre" == "$2" ]] && v2_pre="" || v2_pre="${v2_pre%+*}"
+
local IFS=.
- local i ver1=($1) ver2=($2)
+ local i ver1=($v1_core) ver2=($v2_core)
# ... rest of the functionLikely invalid or redundant comment.
| ```bash | ||
| mkdir -p ~/.local/bin | ||
| curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/ivuorinen/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup | ||
| chmod +x ~/.local/bin/x-asdf-cleanup | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance installation instructions for better security and reliability.
The installation process should include:
- Checksum verification for the downloaded script
- Error handling for directory creation
- Download validation
```bash
-mkdir -p ~/.local/bin
-curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/ivuorinen/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup
-chmod +x ~/.local/bin/x-asdf-cleanup
+# Create directory with error handling
+if ! mkdir -p ~/.local/bin; then
+ echo "Error: Failed to create directory ~/.local/bin"
+ exit 1
+fi
+
+# Download script and verify
+SCRIPT_URL="https://raw.githubusercontent.com/ivuorinen/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup"
+TEMP_SCRIPT=$(mktemp)
+if ! curl -fsSL "$SCRIPT_URL" -o "$TEMP_SCRIPT"; then
+ echo "Error: Failed to download script"
+ rm -f "$TEMP_SCRIPT"
+ exit 1
+fi
+
+# Move to final location and set permissions
+if ! mv "$TEMP_SCRIPT" ~/.local/bin/x-asdf-cleanup; then
+ echo "Error: Failed to install script"
+ rm -f "$TEMP_SCRIPT"
+ exit 1
+fi
+chmod +x ~/.local/bin/x-asdf-cleanup| 4. Run tests: | ||
|
|
||
| ```bash | ||
| shellcheck x-asdf-cleanup | ||
| shfmt -d x-asdf-cleanup | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance testing framework with comprehensive test coverage.
The current testing approach only covers linting. Consider adding:
- Unit tests for core functionality
- Integration tests for version management
- Test cases for error scenarios
Would you like me to help create a comprehensive test framework with example test cases? Based on the learnings, we can create a single test file named x-asdf-cleanup.tests.sh.
| ## Features | ||
|
|
||
| - Scans for version files (`.tool-versions`, `.nvmrc`, `.python-version`) | ||
| - Detects installed versions that are no longer in use | ||
| - Supports dry-run mode for safe verification | ||
| - Parallel processing for better performance | ||
| - Built-in caching system | ||
| - Automatic backups before uninstallation | ||
| - Restore functionality for accidental removals | ||
| - Comprehensive logging | ||
| - Progress indication during operations | ||
| - Performance metrics |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security considerations section.
Given that this tool performs system modifications, consider adding a section on security that covers:
- Risks of automatic version removal
- Backup security and retention policy
- Permission requirements
- Safe handling of system-wide changes
| # Uninstall versions with progress indication | ||
| uninstall_versions() | ||
| { | ||
| if ((${#uninstall_list[@]} == 0)); then | ||
| print_success "No versions to uninstall." | ||
| return | ||
| fi | ||
|
|
||
| if [[ $DRYRUN == true ]]; then | ||
| print_warning "Dry run mode - would uninstall these versions:" | ||
| for key in "${uninstall_list[@]}"; do | ||
| echo " Would uninstall: $key" | ||
| done | ||
| log_to_file "INFO" "Dry run completed, would uninstall ${#uninstall_list[@]} version(s)" | ||
| return | ||
| fi | ||
|
|
||
| local confirm | ||
| read -rp "Do you want to proceed with uninstallation? (y/N): " confirm | ||
| if [[ $confirm =~ ^[Yy]$ ]]; then | ||
| local total=${#uninstall_list[@]} | ||
| local current=0 | ||
|
|
||
| for key in "${uninstall_list[@]}"; do | ||
| ((current++)) | ||
| local name="${key% *}" | ||
| local version="${key#* }" | ||
| print_success "($current/$total) Uninstalling $name: $version" | ||
|
|
||
| # Create backup before uninstalling | ||
| if ! create_backup "$name" "$version"; then | ||
| print_warning "Failed to create backup for $name $version" | ||
| continue | ||
| fi | ||
|
|
||
| if ! asdf uninstall "$name" "$version"; then | ||
| print_error "Failed to uninstall $name $version" | ||
| log_to_file "ERROR" "Failed to uninstall $name $version" | ||
| else | ||
| log_to_file "INFO" "Successfully uninstalled $name $version" | ||
| fi | ||
| done | ||
| print_success "Uninstallation completed" | ||
| log_to_file "INFO" "Uninstallation process completed" | ||
| else | ||
| print_warning "Uninstallation aborted by user." | ||
| log_to_file "INFO" "Uninstallation aborted by user" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance uninstallation error handling and add rollback mechanism.
The uninstallation process should handle errors more gracefully and support rollback.
uninstall_versions() {
+ local -a failed_uninstalls=()
+ local rollback_needed=false
+
# ... existing code ...
for key in "${uninstall_list[@]}"; do
((current++))
local name="${key% *}"
local version="${key#* }"
print_success "($current/$total) Uninstalling $name: $version"
# Create backup before uninstalling
if ! create_backup "$name" "$version"; then
print_warning "Failed to create backup for $name $version"
+ failed_uninstalls+=("$key")
continue
fi
if ! asdf uninstall "$name" "$version"; then
print_error "Failed to uninstall $name $version"
log_to_file "ERROR" "Failed to uninstall $name $version"
+ failed_uninstalls+=("$key")
+ rollback_needed=true
+ break
else
log_to_file "INFO" "Successfully uninstalled $name $version"
fi
done
+ if [[ $rollback_needed == true ]]; then
+ print_warning "Uninstallation failed, rolling back changes..."
+ for key in "${failed_uninstalls[@]}"; do
+ local name="${key% *}"
+ local version="${key#* }"
+ if restore_last_backup "$name" "$version"; then
+ print_success "Rolled back $name $version"
+ else
+ print_error "Failed to roll back $name $version"
+ fi
+ done
+ fi
+
print_success "Uninstallation completed"
log_to_file "INFO" "Uninstallation process completed"
}Committable suggestion skipped: line range outside the PR's diff.
Tool for cleaning up unused and old asdf plugin installations.
Summary by CodeRabbit
New Features
x-asdf-cleanupscript to manage and clean up tool versions installed viaasdfDocumentation
x-asdf-cleanuptoolChores