Make it possible to activate venvs outside VIRTUALFISH_HOME

[urxvtcd]

#223
I took a stab at it. Let me know what you think. Hopefully this doesn't break anything 😄

[justinmayer]

Hi Marcin. Please accept my apologies for the delay in reviewing your pull request. In addition to the in-line comments, I have three more thoughts:

1. I wonder whether we need to restrict the user to relative paths. I think there is potential value in allowing the user to also specify absolute paths. So I think there are three options here:
   (1) Only allow relative paths (what is present in this PR right now).
   (2) Assume paths are absolute.
   (3) Check if argument starts with /, ~/, or $ — if so, assume absolute path. If not, assume relative path.

This latter option seems like a user-friendly choice and perhaps should be straightforward to implement?

2. If the argument doesn’t contain a slash, doesn’t match a virtual environment name in $VIRTUAL_FISH_HOME, but does match a virtual environment name in $PWD, should we detect that and activate it? Perhaps that’s another win in the name of user-friendliness?

3. Today I was working on extending the behavior of vf upgrade in order to operate on virtual environments that are outside of $VIRTUAL_FISH_HOME, and I realized that the logic in this pull request probably should be extracted into a separate function so it can be re-used in multiple contexts and not just for virtual environment activation. Perhaps we create an internal __vfsupport_check_venv function to encapsulate this logic so it can be re-used for activation, upgrades, and perhaps other contexts?

What do you think about these ideas?

[justinmayer]

I think this can be done within Fish and without piping to grep:

if string match "*/*" $argv[1]

Unless I'm reading the following output wrong, I think doing this operation using Fish's string match is nearly three orders of magnitude faster:

~ ➤ time string match "*/*" "~/.virtualenvs"

________________________________________________________
Executed in    5.00 micros    fish           external
   usr time    4.00 micros    4.00 micros    0.00 micros
   sys time    2.00 micros    2.00 micros    0.00 micros

~ ➤ time echo "~/.virtualenvs" | grep -q /

________________________________________________________
Executed in    3.75 millis    fish           external
   usr time    0.89 millis  184.00 micros    0.71 millis
   sys time    2.46 millis  766.00 micros    1.70 millis

[urxvtcd]

No worries, my branch has served my needs well, and I underdstand you're doing this basically for free, no need to apologize :)

Your suggestions seem pretty neat. If you're not in a hurry I'll try to take a stab at them in few days, as lately I've been more busy than usual.

[urxvtcd]

Sorry for the delay. I incorporated your suggestions. The logic is encapsulated in the new function. Now it's possible to activate virtualenvs inside $VIRTUALFISH_HOME, and in directiories pointed by absolute and relative paths, including those directly in $PWD (no slashes required). To sum up, the following work, and do what one would expect:

vf activate ./venv
vf activate /home/projects/foo/.venv
vf activate .
vf activate ../venv/

One thing I'm slightly concerned about is activating a virtualenv in $PWD as a security issue, but since $VIRTUALFISH_HOME is checked before the $PWD, I don't think this is a real problem.

If you have any further comments, I'll be happy to address them, with smaller latency this time I hope, haha.

[urxvtcd]

I noticed that the feature doesn't play well with paths containing spaces. From what I understand, fixing that would require making some changes to the vf function. I might look into that.