Skip to content

Conversation

@Francesco146
Copy link

Since it's a big change, i'll open multiple PR to simplify the review process.

Linked to the old: #685

Copilot AI review requested due to automatic review settings December 20, 2025 07:38
Copy link
Contributor

Copilot AI left a 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.

Comment on lines +508 to +509
import psutil
parent = psutil.Process().parent()
Copy link

Copilot AI Dec 20, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines +501 to +503
if "fish" in shell_path:
return "fish"
elif "bash" in shell_path or "zsh" in shell_path or "sh" in shell_path:
Copy link

Copilot AI Dec 20, 2025

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.

Suggested change
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"):

Copilot uses AI. Check for mistakes.

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')}")
Copy link

Copilot AI Dec 20, 2025

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.

Suggested change
console.print(f"[dim]Detected shell:[/dim] {os.environ.get('SHELL', 'unknown')}")
console.print(f"[dim]Detected shell type:[/dim] {selected_script}")

Copilot uses AI. Check for mistakes.
"""Detect the current shell environment.
Returns:
Shell type identifier: 'fish', 'sh' (for bash/zsh), or 'ps' (PowerShell)
Copy link

Copilot AI Dec 20, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant