🧹 Refactor: Extract duplicated version classification logic into shared helper#44
🧹 Refactor: Extract duplicated version classification logic into shared helper#44savvides wants to merge 1 commit into
Conversation
- Extracted the duplicate version classification case statement from `setup` and `bin/idstack-doctor` into a new `bin/idstack-version-classifier` script. - Updated `setup` and `bin/idstack-doctor` to execute the new helper. - Updated `test/test-version-classifier.sh` to test the new helper directly rather than duplicating the logic inline. - Verified all tests pass correctly. Co-authored-by: savvides <1580637+savvides@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the version classification logic by extracting it into a new shared helper script, bin/idstack-version-classifier, which is now used by setup, bin/idstack-doctor, and the test suite. The feedback suggests addressing an edge case where the version 2.0 is not correctly classified as legacy, and recommends invoking the helper script with bash rather than executing it directly to prevent potential permission issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| case "${1:-}" in | ||
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|[1-9][0-9]*) echo "skip" ;; | ||
| 0.*|1.*|2.0.0.*|2.0.0) echo "legacy" ;; |
There was a problem hiding this comment.
The version 2.0 is a legacy version (pre-v2.0.1.0) but it currently does not match any of the patterns, resulting in an unknown classification. This means a legacy installation with version 2.0 in its VERSION file will not be detected or cleaned up during setup.
Adding 2.0 to the legacy pattern ensures it is correctly classified.
Please also consider adding check "2.0" legacy to the test cases in test/test-version-classifier.sh to prevent regressions.
0.*|1.*|2.0.0.*|2.0.0|2.0) echo "legacy" ;;
| esac | ||
| # Uses shared classifier to identify pre-v2.0.1.0 versions. Mirrors setup. | ||
| # Covered by test/test-version-classifier.sh. | ||
| class=$("$IDSTACK_DIR/bin/idstack-version-classifier" "$v") |
There was a problem hiding this comment.
Executing the helper script directly relies on the file having the executable permission bit set (chmod +x). In some environments (e.g., Windows/WSL, or when files are transferred/cloned with certain git configurations), the executable bit can be lost, leading to a "Permission denied" error.
Invoking the script with bash is more robust and avoids permission issues.
class=$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$v")
| esac | ||
| # Uses shared classifier to identify pre-v2.0.1.0 versions. Covered by | ||
| # test/test-version-classifier.sh. | ||
| class=$("$IDSTACK_DIR/bin/idstack-version-classifier" "$legacy_version") |
There was a problem hiding this comment.
Executing the helper script directly relies on the file having the executable permission bit set. Since setup runs with set -e (line 2), any execution failure (such as "Permission denied" if the executable bit is missing) will cause the entire setup process to crash and exit immediately.
Invoking the script with bash is more robust and prevents potential installation failures.
class=$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$legacy_version")
| local expected="$2" | ||
| local got | ||
| got="$(classify_version "$version")" | ||
| got="$("$IDSTACK_DIR/bin/idstack-version-classifier" "$version")" |
There was a problem hiding this comment.
For consistency and robustness, invoke the helper script using bash here as well.
| got="$("$IDSTACK_DIR/bin/idstack-version-classifier" "$version")" | |
| got="$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$version")" |
1 participant
🎯 What: Extracted the duplicate version classification
casestatement fromsetup,bin/idstack-doctor, andtest/test-version-classifier.shinto a new, shared helper script located atbin/idstack-version-classifier.💡 Why: Reduces duplicate logic across multiple standalone scripts. By centralizing the complex bash globs for version strings into a single script, any future additions or changes to version parsing will only need to be updated in one place.
✅ Verification: Validated that both
test/test-version-classifier.shand the full suite (test/smoke-test.shandtest/integration-test.sh) pass perfectly with no regressions. Code review confirmed the safety and correctness of executing a shared helper rather than repeating the case statement.✨ Result: Improved codebase maintainability by establishing a single source of truth for version legacy checking without breaking existing functionality or setup requirements.
PR created automatically by Jules for task 11982650350858743443 started by @savvides