-
Notifications
You must be signed in to change notification settings - Fork 5k
feat(shell): initial Fish support #1358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds initial support for the Fish shell to the Specify CLI by introducing automatic shell detection and Fish as a script type option. The main goal is to allow users with Fish shell to automatically select the appropriate template variant during project initialization.
- Adds a new
detect_shell()function to automatically identify the user's current shell environment - Updates script type choices to include Fish alongside existing POSIX shell (bash/zsh) and PowerShell options
- Reorganizes imports to follow a more consistent alphabetical ordering pattern
Comments suppressed due to low confidence (1)
src/specify_cli/init.py:516
- 'except' clause does nothing but pass and there is no explanatory comment.
except ImportError:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import psutil | ||
| parent = psutil.Process().parent() |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The psutil package is imported but not listed in the project dependencies. This will cause an ImportError for users who don't have psutil installed, preventing the fallback shell detection from working properly. Either add psutil to the dependencies in pyproject.toml, or handle the case where the import fails more gracefully by catching the exception at import time rather than during execution.
| if "fish" in shell_path: | ||
| return "fish" | ||
| elif "bash" in shell_path or "zsh" in shell_path or "sh" in shell_path: |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell detection logic on line 503 may produce false positives. The check "sh" in shell_path will match many unintended paths like "/usr/bin/bash", "/bin/zsh", or even paths containing "sh" anywhere (e.g., "/home/shelly/fish"). The condition should use .endswith("sh") or a more precise pattern match to avoid incorrectly detecting bash/zsh as generic POSIX shell, and to prevent false positives from paths containing "sh" as a substring.
| if "fish" in shell_path: | |
| return "fish" | |
| elif "bash" in shell_path or "zsh" in shell_path or "sh" in shell_path: | |
| shell_name = os.path.basename(shell_path).lower() | |
| if shell_name == "fish": | |
| return "fish" | |
| elif shell_name in ("bash", "zsh", "sh"): |
|
|
||
| console.print(f"[cyan]Selected AI assistant:[/cyan] {selected_ai}") | ||
| console.print(f"[cyan]Selected script type:[/cyan] {selected_script}") | ||
| console.print(f"[dim]Detected shell:[/dim] {os.environ.get('SHELL', 'unknown')}") |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug output shows the raw SHELL environment variable value rather than the detected shell type. This creates confusion as it displays "Detected shell: /usr/bin/fish" instead of "Detected shell: fish". Consider either changing the label to "Shell path" or displaying the detected shell type (the value of selected_script or default_script) to make this output more helpful for debugging.
| console.print(f"[dim]Detected shell:[/dim] {os.environ.get('SHELL', 'unknown')}") | |
| console.print(f"[dim]Detected shell type:[/dim] {selected_script}") |
| """Detect the current shell environment. | ||
| Returns: | ||
| Shell type identifier: 'fish', 'sh' (for bash/zsh), or 'ps' (PowerShell) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring states the function returns 'fish', 'sh', or 'ps', but line 503 also checks for "bash" and "zsh" in the shell path. While these do eventually return "sh", the implementation comment suggests these are distinct shells being detected. Consider clarifying in the docstring that bash and zsh are detected and mapped to the "sh" return value for consistency.
| Shell type identifier: 'fish', 'sh' (for bash/zsh), or 'ps' (PowerShell) | |
| Shell type identifier: | |
| - 'fish' for the fish shell | |
| - 'sh' for POSIX-style shells (e.g. bash, zsh, sh) | |
| - 'ps' for PowerShell / pwsh |
Since it's a big change, i'll open multiple PR to simplify the review process.
Linked to the old: #685