这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@mdeff
Copy link
Contributor

@mdeff mdeff commented Mar 28, 2018

This PR adds autocompletion for conda env to the fish shell.

@mdeff mdeff requested a review from a team as a code owner March 28, 2018 17:12
@mdeff
Copy link
Contributor Author

mdeff commented Mar 29, 2018

CC @jaimergp and @kalefranz who worked on #6885 and #6502

Copy link
Member

@mbargull mbargull left a 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
Copy link
Member

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
  end

That 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.

Copy link
Contributor Author

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.

Copy link
Member

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.)

Copy link
Contributor Author

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. :)

Copy link
Member

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 😆

Copy link
Contributor Author

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 😆

@mdeff
Copy link
Contributor Author

mdeff commented Apr 2, 2018

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 conda_env/shell/etc/fish/conf.d/conda_env.fish but thought it was a bit absurd to create this whole hierarchy for some lines of fish shell (plus we'd have to insert the _CONDA_EXE and _CONDA_ROOT variables in this file at install time). Let me know if you think it's preferable this way and I'll move it.

@jaimergp
Copy link
Contributor

jaimergp commented Apr 2, 2018

LGTM, but I haven't tested it yet. Do you need me to check it today?

@mbargull mbargull added the cli pertains to the CLI interface label Apr 2, 2018
@jaimergp
Copy link
Contributor

jaimergp commented Apr 7, 2018

Quick update: I have replaced my system's conda.fish with this proposal and works perfect.

@mdeff
Copy link
Contributor Author

mdeff commented Apr 11, 2018

Thanks @jaimergp 👍

@mbargull can we merge?

@kalefranz kalefranz merged commit 3f556de into conda:master Apr 13, 2018
@kalefranz
Copy link
Contributor

Thanks for the PR!

@github-actions
Copy link

github-actions bot commented Sep 3, 2021

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.

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Sep 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cli pertains to the CLI interface locked [bot] locked due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants