Conversation
Summary of ChangesHello, 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 installation experience by providing helpful, non-blocking nudges to users regarding 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. 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 introduces functionality to detect if tmux is installed and, if not, provides installation hints to the user. This functionality is implemented in both install.sh and src/install.ts. The review comments highlight that the platform detection logic in install.sh is incomplete, potentially causing the installer to fail on some Unix-like systems. Additionally, the tmuxInstallHint function in src/install.ts could be improved to offer more specific installation advice for a wider range of Unix-like platforms and should include corresponding test cases.
| tmux_install_hint() { | ||
| if [ "$os" = "macos" ]; then | ||
| echo "brew install tmux" | ||
| return | ||
| fi | ||
|
|
||
| echo "your package manager (for example: apt install tmux)" | ||
| } |
There was a problem hiding this comment.
This new function relies on the $os variable. It's worth noting that the case statement that sets $os on lines 52-56 only handles Darwin and Linux. For other Unix-like systems (e.g., FreeBSD), it will fall back to the default case and exit the script with a 'Windows is not supported' error. This prevents the installer from running at all on those platforms.
While the fix is outside of this diff, it's a significant correctness issue affecting this new functionality. The script should be updated to correctly identify other Unix-like systems to ensure the installer works on all supported platforms.
| const tmuxInstallHint = ( | ||
| platform: NodeJS.Platform = process.platform | ||
| ): string => { | ||
| if (platform === "darwin") { | ||
| return TMUX_MACOS_HINT; | ||
| } | ||
| if (platform === "linux") { | ||
| return TMUX_LINUX_HINT; | ||
| } | ||
| return "install tmux"; | ||
| }; |
There was a problem hiding this comment.
This function provides helpful installation hints. To improve support for other Unix-like systems, you could provide the generic Linux hint for platforms like FreeBSD and OpenBSD, instead of the less helpful default. A switch statement could make this cleaner.
Also, consider adding test cases for these new platforms in tests/install.test.ts.
const tmuxInstallHint = (
platform: NodeJS.Platform = process.platform
): string => {
switch (platform) {
case "darwin":
return TMUX_MACOS_HINT;
case "linux":
case "freebsd":
case "openbsd":
case "sunos":
return TMUX_LINUX_HINT;
default:
return "install tmux";
}
};
1 participant
Summary
bun run install:globalVerification