feat: Add rbenv option for Ruby installation #34
Conversation
|
HarperBot AnalysisError generating analysis: API quota exceeded (PR: feat: Add rbenv option for Ruby installation, Model: gemini-2.0-flash, Diff length: 681). Please check your billing or try again later. |
Summary of ChangesHello @bniladridas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds a useful feature to allow choosing between a direct Homebrew Ruby installation and using rbenv for version management. The implementation has a few critical issues in the rbenv installation logic that will cause the script to fail. Specifically, it misses the Ruby installation step via rbenv install and doesn't initialize rbenv for the current script session. I've provided a detailed comment with a code suggestion that fixes these issues, improves the code structure by using a case statement to avoid duplication, and adds error handling for consistency with the rest of the script.
setup-mac.sh
Outdated
| read -r -p "Choose Ruby installation method (1 for direct brew install, 2 for rbenv): " ruby_choice | ||
| if [ "$ruby_choice" = "1" ]; then | ||
| brew install ruby | ||
| elif [ "$ruby_choice" = "2" ]; then | ||
| brew install rbenv | ||
| # shellcheck disable=SC2016 | ||
| echo 'eval "$(rbenv init -)"' >> ~/.zshrc | ||
| export PATH="/opt/homebrew/bin:$PATH" | ||
| rbenv global 3.2.2 | ||
| else | ||
| echo "Invalid choice, installing Ruby directly." | ||
| brew install ruby | ||
| fi |
There was a problem hiding this comment.
The rbenv installation logic has a few critical issues and can be improved for clarity and robustness.
-
Broken
rbenvworkflow: The current logic will fail because:- It tries to set a global Ruby version (
rbenv global 3.2.2) without first installing it viarbenv install 3.2.2. rbenvcommands won't work correctly in the current script session becauserbenvis not initialized. Theecho ... >> ~/.zshrconly configures future shell sessions. You need to runeval "$(rbenv init -)"within the script.- The
export PATHon line 105 is not the correct way to configure the path forrbenv; this is handled byrbenv init.
- It tries to set a global Ruby version (
-
Code Structure: The
if/elif/elseblock duplicates thebrew install rubycommand. Using acasestatement is a cleaner way to handle multiple choices and improves maintainability. -
Missing Error Handling: For consistency with other installation steps in this script (like Python and Node.js), it's good practice to add explicit error handling for the installation commands.
Here is a suggested replacement that addresses these points:
read -r -p "Choose Ruby installation method (1 for direct brew install, 2 for rbenv): " ruby_choice
case "$ruby_choice" in
2)
log "Installing rbenv..."
if ! brew install rbenv; then
log "ERROR: Failed to install rbenv."
exit 1
fi
# Add rbenv to zsh for future sessions
# shellcheck disable=SC2016
echo 'eval "$(rbenv init -)"' >> ~/.zshrc
# Initialize rbenv for the current script session
eval "$(rbenv init -)"
log "Installing Ruby 3.2.2 with rbenv (this may take a while)..."
if rbenv install 3.2.2; then
rbenv global 3.2.2
log "Ruby 3.2.2 installed and set as global via rbenv."
else
log "ERROR: Failed to install Ruby 3.2.2 with rbenv."
exit 1
fi
;;
*)
if [ "$ruby_choice" != "1" ]; then
echo "Invalid choice, installing Ruby directly."
fi
if brew install ruby; then
log "Ruby installed directly via Homebrew."
else
log "ERROR: Failed to install Ruby."
exit 1
fi
;;
esac
|
HarperBot AnalysisError generating analysis: API quota exceeded (PR: feat: Add rbenv option for Ruby installation, Model: gemini-2.0-flash, Diff length: 1294). Please check your billing or try again later. |
Summary