justinmayer / virtualfish

Fish shell tool for managing Python virtual environments
MIT License
1.06k stars 100 forks source link

Improve compat aliases deactivate #154

Closed GuyShane closed 4 years ago

GuyShane commented 4 years ago

Move definition of the deactivate function inside workon, so it is only defined when using a virtual environment. Then when deactivate is called, it removes itself from the session.

justinmayer commented 4 years ago

Thanks for your contribution submission, Shane. Two thoughts…

  1. I'm not sure what problem this change is intended to solve. Perhaps you can elaborate?

  2. This change breaks functionality upon which my workflow depends. Namely, being able to run deactivate when the virtual environment in question was activated by something other than manually invoking workon. For example:

    ~ ➤ vf tmp -p python3
    (tempenv-4d1a11231948) ~ ➤ [… do things in temporary environment… ]
    (tempenv-4d1a11231948) ~ ➤ deactivate
    fish: Unknown command: deactivate
    fish:
    deactivate
    ^

    … and then I am sad. 😞

Of course, I could run vf deactivate instead, but that's extra typing and at this point goes against years of muscle-memory.

GuyShane commented 4 years ago

Ah okay, sorry about that! I was hoping to more closely replicate the virtualenv behaviour where deactivate is only defined when there is something active. I wrote a similar utility for activating docker machines and it uses a temporary deactivate function too, so I didn't want to have a deactivate function permanently defined. But I don't want to break your workflow! I have this change in my local copy and that's working for me

justinmayer commented 4 years ago

No worries. I'm not opposed to the proposed functionality itself, provided that it behaves consistently regardless of how the virtual environment was activated. I imagine that would involve changes to the core activate and deactivate functions, as opposed to only changing the compat_aliases plugin.

justinmayer commented 4 years ago

Thanks again for your contribution, Shane. I appreciate what you were trying to accomplish in this pull request, so I came up with a solution that should be consistent regardless of how virtual environments are activated.

Thanks to b6dd00f0c1df3df1cb46a173786db1b4d85c4599, the deactivate alias is only defined when the virtualenv_did_activate event fires, and the alias is undefined when the virtualenv_did_deactivate event fires. This feature is included in the just-shipped VirtualFish 2.1 release. ✨