-
Notifications
You must be signed in to change notification settings - Fork 2k
fish autocompletion for conda env #7101
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
Conversation
|
CC @jaimergp and @kalefranz who worked on #6885 and #6502 |
mbargull
left a comment
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 assume those changes are fine (apart from the one comment I made and given my limited fish knowledge).
Although, as a general note, conda-env is actually somewhat separated from conda itself and these changes would bring in conda-env specifics into conda's code.
Well, of course we are talking about a very specialized and small case, thus I don't care very much myself (and because I neither use fish nor conda-env 😜) -- let's see what @kalefranz thinks about it.
| string replace -r '.*_([a-z]+)\.py$' '$1' $_CONDA_ROOT/lib/python*/site-packages/conda/cli/main_*.py | ||
| echo activate | ||
| echo deactivate | ||
| echo env |
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 think instead of echo env we should do the more general
string replace -r '^.*/conda-' '' "$_CONDA_ROOT"/bin/conda-*or maybe better
for f in "$_CONDA_ROOT"/bin/conda-*
if test -x "$f" -a ! -d "$f"
string replace -r '^.*/conda-' '' "$f"
end
endThat way we would also pick up other commands, e.g., from conda-build.
NOTE: I have next to no fish experience, so I don't know certainly if the code snippet above is fine or not.
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.
Good idea! I think the first is fine. We don't have conda directories AFAIK, and we might discover other things to exclude later anyway.
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 was more thinking about the test -x part, i.e., to only pick up executables. The -d part is only to a necessary addition.
If one would do something like writing a (of course non-executable) conda-build.log file to bin or another stupidity, then we can easily exclude that with the second code snippet.
(BTW, conda itself doesn't do that executable test yet and thus will ungracefully exit if you were to run it with such conda conda-build.log -- I'll submit a PR for this.)
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.
Looks strange to me to have non-executable files in bin but ok, done. :)
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.
Definitely agree to that. But who knows what people might do 😉 -- we can always remove that check later on.
One case where we have non-executables would be on Windows where (due to no shebang support) a single conda script is replaced by conda.exe and conda-script.py (with the former essentially running python conda-script.py).
But well, to satisfy the "immensely large" Windows+fish+conda user base, we'd have to glob for conda-*(.exe)? or something 😆
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.
Hahaha this immensely large user base will certainly submit a PR if there's an issue 😆
|
Thanks for your review @mbargull. I've implemented your suggestion. I too thought about keeping the separation between conda and conda-env by putting these rules in |
|
LGTM, but I haven't tested it yet. Do you need me to check it today? |
|
Quick update: I have replaced my system's |
|
Thanks for the PR! |
|
Hi there, thank you for your contribution to Conda! This pull request has been automatically locked since it has not had recent activity after it was closed. Please open a new issue or pull request if needed. |
This PR adds autocompletion for
conda envto the fish shell.