-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature/autocompletion #29
base: master
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.
I'm not a maintainer, but this looks great to me and seems like a good idea.
bin/install
Outdated
if [ $config_vercomp == "ge" ]; then | ||
if echo "$SHELL" | grep -q "bash"; then | ||
if [ ! -f "$HOME/.bash_completion" ]; then | ||
echo "Detected default shell bash" |
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.
Multiple instances: consider "bash" --> "Bash" since it's a proper noun.
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.
Gone through and replaced any instance of bash or zsh with Bash/Zsh as that appears to be the correct way. From what I could find of fish shell it seems to always appear as lowercase so have left that the way it is.
bin/install
Outdated
"$install_path"/bin/poetry completions fish >~/.config/fish/completions/poetry.fish | ||
fi | ||
else | ||
echo "Warning: Could not detect BASH, ZSH or FISH." |
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.
Consider "BASH, ZSH or FISH" --> "Bash, zsh, or fish shells" since new developers may not realize what these terms refer to and assume they are acronyms.
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.
Taken your comment on board. As a compromise are you happy with Could not detect shells Bash, Zsh or fish
? As you pointed out, a new dev may not realise what they are and with shells
at the end may think fish shells
is one thing.
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.
Yeah, I find your proposed rewording the clearest.
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.
Yeah, I find your proposed rewording the clearest.
bin/install
Outdated
echo "Could not complete autocompletion setup" | ||
fi | ||
else | ||
echo "Warning: Poetry versions prior to 1.2.0 will not have autocompletion" |
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.
Nit: "will" --> "do"
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.
Yep. That sounds better to me. Changed.
Testing in a container with bash as the default shell it seems the completion works but with some issues. Here are my findings. Found .bash_completions isn't sourced in the bashrc file by default and I needed to add it which isn't in the poetry instructions. As someone that's been a Zsh user for some time, I don't know if this is the norm or possibly something to do with the fact I was using a container and setup to be barebones. So would be good to get some input on that if anyone knows? Poetry installs and detects default shell Bash:
Then source Bash and get the following error:
This appears to be a known issue and is fixed, but I'm assuming in the next release: python-poetry/poetry#6577 I run the sed command mentioned in the issue, and although kind of works, found another issue:
Which lead me to this issue: python-poetry/poetry#3418 But after installing that package, I get the following result:
|
Thought I'd give #27 a crack.
Based changes off the following documentation: https://python-poetry.org/docs/#enable-tab-completion-for-bash-fish-or-zsh
Full disclosure. I was able to fully test with zsh and worked great. I did try setting my default shell to bash/fish, but came across issues just creating a new terminal session so was unable to test the bash/fish side running
asdf install poetry 1.2.2
.I'll try creating a container, installing asdf with bash/fish over the next few days and check the results. But wanted to create this PR as I'm sure the functionality is there and wanted this change to be open for discussion.