-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Check end of path for shell detection #6212
Conversation
Addresses issue: pypa#6197 Previously these branches would trip if the shell name was present anywhere in the cmd path. This updates the check to only look at the end.
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.
Actually, I prefer this over the split over .linuxbrew
.
This solution will cause future maintainers to raise eyebrows and wrinkle their foreheads trying to figure it out without reading the original issue.
This approach is much more maintainable. However, I have a small suggestion:
Change:
cmd.endswith("<shell name>")
to:
cmd.endswith("/<shell name>"):
This will allow to for shortcutting the if ...elif at the head in favor of the most common shells (bash, zsh, sh):
if cmd.endswith("/sh","/bash","/zsh")":
suffix = ""
command = "."
elif cmd.endswith("/csh"):
...
else:
raise ValueError("unknown shell ...") ....
This works for all three:
>>> cmd="/bin/sh"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>> cmd="/bin/zsh"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>> cmd="/bin/bash"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>>
@JoshStern thank you for your effort so far (immediately adding news file, is great! ). I gave your PR some consideration and I think that your solution is better than what I initially suggested. |
@oz123 updated! Thank you for the feedback and quick turnaround. Want me to also update the branch ordering? |
Yes. Please. The zsh, bash and sh bash should be at the top. Unknown shells should not fall silently to imaginary default. They should raise exceptions. |
Updates the if else block to only allow an explicit set of shells instead of defaulting to the `activate` script.
Updated, I'm a little concerned about other I do think an explicit list is better though. I defer to you on the right balance. |
Thanks! I merged your commits with one on top! |
Thank you for the help with this! Glad I could contribute 😄 |
Thank you for contributing to Pipenv!
The issue
Shell detection checks the entire path when looking for shell substrings. This can cause the wrong branch to get tripped if shells are installed in a path with another shell in the tree. In my case a
li[nu]xbrew
-installedzsh
goes down thenu
shell path.The fix
This updates the check to only look at the end of
cmd
when detecting shells.The checklist
nu
shell #6197news/
directory to describe this fix with the extension.bugfix.rst
,.feature.rst
,.behavior.rst
,.doc.rst
..vendor.rst
. or.trivial.rst
(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.