feat(bin): x-asdf-cleanup #29
Conversation
|
1 similar comment
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new Bash script Changes
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.
local/bin/x-asdf-cleanup
Outdated
| local fd_command="fd --base-directory $BASE_DIR --glob '.tool-versions' --hidden" | ||
| for pattern in "${EXCLUDE_PATTERNS[@]}"; do | ||
| fd_command="$fd_command --exclude $pattern" | ||
| done | ||
| eval "$fd_command" |
There was a problem hiding this comment.
Avoid using 'eval' for command construction.
Using 'eval' can potentially introduce security risks if BASE_DIR or excluded patterns are dynamically sourced from user input and contain malicious characters. Instead, build an array of arguments and pass it to 'fd' directly without eval.
- eval "$fd_command"
+ $fd_commandCommittable suggestion skipped: line range outside the PR's diff.
local/bin/x-asdf-cleanup
Outdated
| # Function to uninstall versions | ||
| # It will prompt the user for confirmation before uninstalling | ||
| # If DRYRUN is set to true, it will only print out the versions that would be uninstalled | ||
| # | ||
| # Usage: uninstall_versions | ||
| # Output: Uninstall the versions in uninstall_list array | ||
| uninstall_versions() { | ||
| if [[ ${#uninstall_list[@]} -gt 0 ]]; then | ||
| if [ "$DRYRUN" = true ]; then | ||
| confirm="y" | ||
| fi | ||
| if [ ! "$DRYRUN" = true ]; then | ||
| read -p "Do you want to proceed with uninstallation? (y/N): " confirm | ||
| fi | ||
| if [[ "$confirm" =~ ^[Yy]$ ]]; then | ||
| for key in "${uninstall_list[@]}"; do | ||
| local name="${key% *}" | ||
| local version="${key#* }" | ||
| if [ "$DRYRUN" = true ]; then | ||
| echo "(?) Dry run: would uninstall $name $version" | ||
| else | ||
| echo "(*) Uninstalling $name: $version" | ||
| asdf uninstall $name $version | ||
| fi | ||
| done | ||
| else | ||
| echo "Uninstallation aborted by user." | ||
| fi | ||
| else |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code review, I can now provide the final response:
Add error handling for asdf uninstall command failures
The current implementation doesn't check the exit status of asdf uninstall. Add error handling to inform users of failures and potentially continue with remaining uninstallations:
if ! asdf uninstall "$name" "$version"; then
echo "(!) Failed to uninstall $name: $version"
# Optionally: exit 1 or continue
fi🔗 Analysis chain
Provide explicit error handling for 'asdf uninstall' failures.
While iterating over the versions to uninstall, if 'asdf uninstall' fails for any reason, the script continues. Consider implementing checks on the exit status of 'asdf uninstall' to inform the user if something fails.
Let me verify if there's any error handling for the asdf uninstall command in the codebase.
Let me try a different approach to get the context of the function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script will search for any code references to asdf uninstall
# and verify if there's error handling already present.
rg -A3 'asdf uninstall'
Length of output: 196
Script:
#!/bin/bash
# Get more context around the uninstall_versions function
ast-grep --pattern 'uninstall_versions() {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Get the content of the file containing uninstall_versions function
cat local/bin/x-asdf-cleanup
Length of output: 9993
|
@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.
local/bin/x-asdf-cleanup
Outdated
| find_tool_versions_files() { | ||
| echo find_version_files ".tool-versions" | ||
| } |
There was a problem hiding this comment.
Fix: Helper functions incorrectly echo command name instead of executing it
The helper functions find_tool_versions_files, find_nvmrc_files, and find_python_version_files are incorrectly echoing the command name instead of executing it.
- echo find_version_files ".tool-versions"
+ find_version_files ".tool-versions"Apply similar fixes to find_nvmrc_files and find_python_version_files.
Also applies to: 196-198, 203-205
| # tms, https://github.com/jrmoulton/tmux-sessionizer | ||
| export TMS_CONFIG_FILE="${XDG_CONFIG_HOME}/tms/config.toml" | ||
|
|
There was a problem hiding this comment.
Remove duplicate export of TMS_CONFIG_FILE
This export statement is already defined earlier in the file and is redundant.
-# tms, https://github.com/jrmoulton/tmux-sessionizer
-export TMS_CONFIG_FILE="${XDG_CONFIG_HOME}/tms/config.toml"
-
|
@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
local/bin/x-asdf-cleanup
Outdated
| parse_arguments() { | ||
| for arg in "$@"; do | ||
| case $arg in | ||
| -h|--help) | ||
| usage | ||
| ;; | ||
| -v|--version) | ||
| version | ||
| ;; | ||
| --base-dir=*) | ||
| BASE_DIR="${arg#*=}" | ||
| shift | ||
| ;; | ||
| --dry-run) | ||
| DRYRUN=true | ||
| shift | ||
| ;; | ||
| --exclude=*) | ||
| EXCLUDE_PATTERNS+=("${arg#*=}") | ||
| shift | ||
| ;; | ||
| --verbose) | ||
| VERBOSE=true | ||
| shift | ||
| ;; | ||
| --debug) | ||
| DEBUG=true | ||
| shift | ||
| debug_info "$@" | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $arg" | ||
| usage | ||
| ;; | ||
| esac | ||
| done | ||
| } |
There was a problem hiding this comment.
Fix argument parsing logic
The shift commands in the parse_arguments function are misplaced and won't work as intended since they're inside the case statement but after the ;;. This means they never execute.
Apply this fix:
parse_arguments() {
for arg in "$@"; do
case $arg in
-h|--help)
usage
- ;;
- -v|--version)
- version
- ;;
- --base-dir=*)
- BASE_DIR="${arg#*=}"
- shift
- ;;
- --dry-run)
- DRYRUN=true
- shift
- ;;
- --exclude=*)
- EXCLUDE_PATTERNS+=("${arg#*=}")
- shift
- ;;
- --verbose)
- VERBOSE=true
- shift
- ;;
- --debug)
- DEBUG=true
- shift
- debug_info "$@"
- ;;
- *)
- echo "Unknown option: $arg"
- usage
- ;;
+ ;;
+ -v|--version)
+ version
+ ;;
+ --base-dir=*)
+ BASE_DIR="${arg#*=}"
+ ;;
+ --dry-run)
+ DRYRUN=true
+ ;;
+ --exclude=*)
+ EXCLUDE_PATTERNS+=("${arg#*=}")
+ ;;
+ --verbose)
+ VERBOSE=true
+ ;;
+ --debug)
+ DEBUG=true
+ debug_info "$@"
+ ;;
+ *)
+ echo "Unknown option: $arg"
+ usage
+ ;;
esac
+ shift
done
}📝 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.
| parse_arguments() { | |
| for arg in "$@"; do | |
| case $arg in | |
| -h|--help) | |
| usage | |
| ;; | |
| -v|--version) | |
| version | |
| ;; | |
| --base-dir=*) | |
| BASE_DIR="${arg#*=}" | |
| shift | |
| ;; | |
| --dry-run) | |
| DRYRUN=true | |
| shift | |
| ;; | |
| --exclude=*) | |
| EXCLUDE_PATTERNS+=("${arg#*=}") | |
| shift | |
| ;; | |
| --verbose) | |
| VERBOSE=true | |
| shift | |
| ;; | |
| --debug) | |
| DEBUG=true | |
| shift | |
| debug_info "$@" | |
| ;; | |
| *) | |
| echo "Unknown option: $arg" | |
| usage | |
| ;; | |
| esac | |
| done | |
| } | |
| parse_arguments() { | |
| for arg in "$@"; do | |
| case $arg in | |
| -h|--help) | |
| usage | |
| ;; | |
| -v|--version) | |
| version | |
| ;; | |
| --base-dir=*) | |
| BASE_DIR="${arg#*=}" | |
| ;; | |
| --dry-run) | |
| DRYRUN=true | |
| ;; | |
| --exclude=*) | |
| EXCLUDE_PATTERNS+=("${arg#*=}") | |
| ;; | |
| --verbose) | |
| VERBOSE=true | |
| ;; | |
| --debug) | |
| DEBUG=true | |
| debug_info "$@" | |
| ;; | |
| *) | |
| echo "Unknown option: $arg" | |
| usage | |
| ;; | |
| esac | |
| shift | |
| done | |
| } |
added snippets!
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.
|
|
||
| # Signal handling and cleanup | ||
| trap 'cleanup' INT TERM | ||
| #trap 'error_handler $?' ERR |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment error handler trap for comprehensive error handling.
The error handler trap is commented out, which means some errors might not be properly caught and handled.
-#trap 'error_handler $?' ERR
+trap 'error_handler $?' ERR📝 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 'error_handler $?' ERR | |
| trap 'error_handler $?' ERR |
| --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" | |
| } |
| 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") | ||
|
|
||
| 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 | ||
| echo "$tool $version" > "$LAST_OP_FILE" | ||
| return 0 | ||
| fi | ||
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add backup directory permission checks and backup integrity verification.
The backup functions 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
}📝 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.
| 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") | |
| 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 | |
| echo "$tool $version" > "$LAST_OP_FILE" | |
| return 0 | |
| fi | |
| fi | |
| return 1 | |
| } | |
| 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 | |
| } |
| read_installed_versions() | ||
| { | ||
| local current_tool="" | ||
| local line | ||
| debug "Reading installed versions from asdf" | ||
|
|
||
| while IFS= read -r line; do | ||
| if [[ $line =~ ^[^[:space:]] ]]; then | ||
| current_tool=$(trim_whitespace "$line") | ||
| continue | ||
| fi | ||
|
|
||
| line=$(trim_whitespace "$line") | ||
| if [[ $line == "No versions installed" ]]; then | ||
| print_warning "No versions installed for $current_tool" | ||
| log_to_file "WARNING" "No versions installed for $current_tool" | ||
| continue | ||
| fi | ||
|
|
||
| # Extract version, removing the asterisk if present | ||
| local version | ||
| if [[ $line =~ ^\* ]]; then | ||
| version=$(trim_whitespace "${line#\*}") | ||
| keep_version["$current_tool $version"]=1 | ||
| debug "Keeping version: $current_tool $version" | ||
| else | ||
| version=$(trim_whitespace "$line") | ||
| fi | ||
|
|
||
| installed_versions["$current_tool $version"]=1 | ||
| debug "Added installed version: $current_tool $version" | ||
|
|
||
| done < <(asdf list) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asdf command failures.
The read_installed_versions 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
if [[ $line =~ ^[^[:space:]] ]]; then
current_tool=$(trim_whitespace "$line")
continue
fi
# ... rest of the function ...
- done < <(asdf list)
+ done <<< "$asdf_output"
}📝 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.
| read_installed_versions() | |
| { | |
| local current_tool="" | |
| local line | |
| debug "Reading installed versions from asdf" | |
| while IFS= read -r line; do | |
| if [[ $line =~ ^[^[:space:]] ]]; then | |
| current_tool=$(trim_whitespace "$line") | |
| continue | |
| fi | |
| line=$(trim_whitespace "$line") | |
| if [[ $line == "No versions installed" ]]; then | |
| print_warning "No versions installed for $current_tool" | |
| log_to_file "WARNING" "No versions installed for $current_tool" | |
| continue | |
| fi | |
| # Extract version, removing the asterisk if present | |
| local version | |
| if [[ $line =~ ^\* ]]; then | |
| version=$(trim_whitespace "${line#\*}") | |
| keep_version["$current_tool $version"]=1 | |
| debug "Keeping version: $current_tool $version" | |
| else | |
| version=$(trim_whitespace "$line") | |
| fi | |
| installed_versions["$current_tool $version"]=1 | |
| debug "Added installed version: $current_tool $version" | |
| done < <(asdf list) | |
| } | |
| 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 || [[ -n $line ]]; do | |
| if [[ $line =~ ^[^[:space:]] ]]; then | |
| current_tool=$(trim_whitespace "$line") | |
| continue | |
| fi | |
| line=$(trim_whitespace "$line") | |
| if [[ $line == "No versions installed" ]]; then | |
| print_warning "No versions installed for $current_tool" | |
| log_to_file "WARNING" "No versions installed for $current_tool" | |
| continue | |
| fi | |
| # Extract version, removing the asterisk if present | |
| local version | |
| if [[ $line =~ ^\* ]]; then | |
| version=$(trim_whitespace "${line#\*}") | |
| keep_version["$current_tool $version"]=1 | |
| debug "Keeping version: $current_tool $version" | |
| else | |
| version=$(trim_whitespace "$line") | |
| fi | |
| installed_versions["$current_tool $version"]=1 | |
| debug "Added installed version: $current_tool $version" | |
| done <<< "$asdf_output" | |
| } |
| 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?
| ## 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
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
|
|
||
| ```bash | ||
| mkdir -p ~/.local/bin | ||
| curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/yourusername/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup |
There was a problem hiding this comment.
Fix the installation URL placeholder
The curl command contains a placeholder URL that needs to be updated with the correct repository path.
-curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/yourusername/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup
+curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/ivuorinen/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup📝 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.
| curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/yourusername/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup | |
| curl -o ~/.local/bin/x-asdf-cleanup https://raw.githubusercontent.com/ivuorinen/dotfiles/main/.dotfiles/local/bin/x-asdf-cleanup |
Tool for cleaning up unused and old asdf plugin installations.
Summary by CodeRabbit
New Features
x-asdf-cleanup, for managing tool versions with user interaction options.TMS_CONFIG_FILEto streamline configuration for the TMS tool.Bug Fixes
TMS_CONFIG_FILE.Documentation
x-asdf-cleanupscript, including usage instructions and configuration options.