Xonsh activate & deactivate #4227

Open
wants to merge 12 commits into
from

Projects

None yet

3 participants

@scopatz
Contributor
scopatz commented Jan 9, 2017

This provides conda activate and deactivate scripts for xonsh. Please let me know if there is anything else that needs to be done here. If you would like to test these locally, these require the improvements in xonsh/xonsh#2072

scopatz added some commits Jan 8, 2017
@scopatz scopatz initial xonsh activate.xsh 4ef676c
@scopatz scopatz added initial xonsh deactivate 2cb7d1e
@scopatz scopatz wrong args 46a8930
@scopatz scopatz some stuff 49944a3
@scopatz scopatz Merge branch 'master' into xonsh-activate 1eb0f4c
@scopatz scopatz initial shell 0e9a8e6
@scopatz scopatz some small fixes for fuzzy matching
558b184
@njalerikson
Contributor

hm fascinating, there's never too many shells ;)

my only 2 cents:

since xonsh is an exotic shell spawning from python we should be able to do some nifty tricks to get the activate/deactivate scripts to properly recognize xonsh or otherwise bourne-shells vs c-shells:

''':'
# 
# bourne- and c- shell detection code that sources the appropriate scripts
#

exit 0
#'''

#
# xonsh detection code that sources the xonsh scripts
#

if the above is possible then it may make sense to rethink the xonsh code a little to make it parallel the code for bourne- and c- shells in order to decrease longterm code maintenance costs

otherwise if xonsh detection cannot be bundled into activate/deactivate then it would make more sense to make it match the design of the fish support (especially since xonsh supports methods)

also it would be ideal to include the $CONDA_INSTALL_PREFIX from #3959

@scopatz
Contributor
scopatz commented Jan 10, 2017

Thanks for the feedback @njalerikson. I think that rewriting activate/deactivate to be both sh- and xonsh-compatible might be too fragile. This is especially true because of the she-bang line, even though it says to ignore it.

I think the right approach is more fish-like. xonsh has an plugins system and so there is natural place for an extension like this to be installed to. I'll try to refactor in the next day or so.

scopatz added some commits Jan 11, 2017
@scopatz scopatz refactored to conda.xsh, like fish 5bf72c9
@scopatz scopatz Merge branch 'master' into xonsh-activate 2831b5a
@scopatz scopatz install hooks for xontrib
8970220
@scopatz
Contributor
scopatz commented Jan 11, 2017

Ok I have updated this to be more like the fish implementation! I don't think that the $CONDA_INSTALL_PREFIX is really needed here since we no longer rely on sourcing de/activate scripts. xonsh will find the hooks in the usual xontrib location without going through the PATH

@scopatz
Contributor
scopatz commented Jan 11, 2017

Any pointers here @njalerikson? The errors seem totally unrelated to anything I added. I have been over it a few times and checked for syntax errors and there don't seem to be any.

@kalefranz
Member

Hey, going to try circling back here soon. 4.3 is about to drop in defaults at any point, and then I'll have some time. There are a lot of failing tests in master right now. Test failures here might not be your fault.

@scopatz
Contributor
scopatz commented Jan 11, 2017

Ahh OK, thanks!

@njalerikson
Contributor

Not an error item but I do see that you have attempted to install the xonsh scripts via the setup.py file.

This is no longer the conda way. conda installs itself using the recipe defined in conda.recipe. The setup.py file exists only to allow the conda library to be installed via pip.

I must admit though that I have not been able to keep up with all of the changes made in the last month regarding the recipe and how conda is installed. Probably the best way to see where things may need to go is to search for references to conda.fish and compare that with references to activate.csh.

scopatz added some commits Jan 13, 2017
@scopatz scopatz Merge branch 'master' into xonsh-activate b2c6e0a
@scopatz scopatz Updated xontrib install to conda.recipe
1873c31
@scopatz
Contributor
scopatz commented Jan 13, 2017

Thanks for the hint @njalerikson. I have updated the PR to reflect this.

@scopatz
Contributor
scopatz commented Jan 17, 2017

Pinging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment