-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Provide platform-specific hint for poetry shell #10177 #10181
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new feature that provides shell-specific hints to the user when the Sequence diagram for poetry shell command with shell detection and hintsequenceDiagram
participant User
participant PoetryApplication
participant Shellingham
participant IO
User->>PoetryApplication: Runs `poetry shell`
PoetryApplication->>Shellingham: detect_shell()
alt Shell detected
Shellingham-->>PoetryApplication: Shell name (e.g., bash)
PoetryApplication->>PoetryApplication: Determines activation command based on shell
PoetryApplication->>IO: Writes error message and activation hint
IO-->>User: Displays error and hint
else Shell detection fails
Shellingham-->>PoetryApplication: ShellDetectionFailure
PoetryApplication->>IO: Writes error message without hint
IO-->>User: Displays error
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @daviewales - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a fallback mechanism if
shellingham
fails to detect the shell, perhaps prompting the user to manually specify their shell type.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
elif shell in ("fish", "elvish"): | ||
env_activate_command = "eval (poetry env activate)" |
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.
question (bug_risk): Double-check the activation command syntax for fish and elvish.
Make sure that the command syntax used here is correct for fish/elvish shells, as they might have specific requirements or variations from other shells.
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.
I checked this. I would appreciate a human double-checking the command syntax however.
except shellingham.ShellDetectionFailure: | ||
shell = None | ||
|
||
if shell in ("sh", "bash", "dash", "ash", "zsh", "ksh", "xonsh"): |
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.
issue (complexity): Consider using a dictionary to map shells to activation commands for improved readability and maintainability.
def env_activate_hint() -> str | None:
try:
import shellingham
shell = shellingham.detect_shell()[0]
except shellingham.ShellDetectionFailure:
shell = None
shell_commands = {
"sh": "eval $(poetry env activate)",
"bash": "eval $(poetry env activate)",
"dash": "eval $(poetry env activate)",
"ash": "eval $(poetry env activate)",
"zsh": "eval $(poetry env activate)",
"ksh": "eval $(poetry env activate)",
"xonsh": "eval $(poetry env activate)",
"csh": "eval `poetry env activate`",
"tcsh": "eval `poetry env activate`",
"fish": "eval (poetry env activate)",
"elvish": "eval (poetry env activate)",
"powershell": "Invoke-Expression (poetry env activate)",
"pwsh": "Invoke-Expression (poetry env activate)",
}
env_activate_command = shell_commands.get(shell)
if env_activate_command:
return (
f"<b>HINT</b>: Your shell appears to be {shell}. You probably need this command:\n\n"
f"<c1>{env_activate_command}</>"
)
return None
Actionable Steps:
- Replace the multiple
if/elif
branches with a mapping (shell_commands
) to associate shells with commands. - Use the
get
method on the dictionary to retrieve the command based onshell
, which reduces duplicate logic and improves maintainability.
These changes keep functionality intact while simplifying the code logic.
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.
I'm not convinced that this is better. There is a lot of duplication in the dictionary version. You could deduplicate the dictionary with variables, but I think the if-else approach is OK.
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.
Thank you for your feedback, we will generate fewer comments like this in the future.
A fall-back is not necessary, because this is an extension to the existing help message. It automatically falls back to the existing message if the shell is not detected. |
92c3e8d
to
65dca15
Compare
I don't like it. I was against having the previous help message about |
Pull Request Check List
Resolves: #10177
I'm not sure if this needs a documentation change.
Let me know if it needs a test and I'll add one.