Add environment plugin #119

zsommers · 2017-04-18T06:09:19Z

Add environment plugin and documentation

This is built largely on the existing work of shanx in #39 and the ideas in #92 by adambreneck. This should address #92 and my manual testing was successful. Please let me know if there are any improvements that I can make.

Add environment plugin and documentation

daisylb · 2017-04-19T01:07:57Z

Hi Zach,

Thanks for this! Looks really good. I'll test and merge soon.

zsommers · 2017-04-19T02:43:39Z

I totally forgot to activate the env vars after editing them, so here's an update that fixes that.

justinmayer · 2017-04-19T04:37:22Z

Glad this is making its way into Virtualfish. I'll review as well and provide some comments shortly.

daisylb · 2017-04-22T10:39:30Z

Okay, just had a look at it.

Everything I tested works great 👍 except for one issue: when I add blank lines to virtualfish-environment, I get the error message set: Variable name can not be the empty string, followed by usage info for set.

While I'm on it, something I just thought of now: it would also be good if comments starting with # could be supported; they currently produce a similar result to the above. Just testing if the first character of a line is # should do the trick, no need for anything more than that imo.

Oh, one last thing: The syntax of the virtualfish-environment file should probably be specified in the docs, too, maybe with an example file listing.

Add ability to parse empty lines and comments in the `virtualfish-environment` file Added description of `virtualfish-environment` file format to docs

zsommers · 2017-04-22T23:21:49Z

@adambrenecki I added parsing for comments and empty lines along with updating the docs. Let me know if you see any other improvements that would be good. Thanks!

zsommers · 2017-05-04T23:13:15Z

@adambrenecki Were there any other potential improvements that you spotted?

justinmayer · 2017-05-04T23:25:57Z

@zsommers: Sorry for the delay in reviewing, Zach. I've set aside time tomorrow to do some in-depth testing. Thanks for your patience!

zsommers · 2017-05-05T02:13:45Z

@justinmayer No worries at all, just wanted to check in =)

justinmayer · 2017-05-05T18:38:41Z

After taking a look, I have to congratulate you, Zach, for some really nice work on this plugin. One of the main reasons I wanted to review this in detail is that I wrote my own implementation back in January but haven't had time to clean it up and submit it. The good news is that your pull request elegantly handles the following deficiencies in my implementation, which were on my to-do-someday list:

record existing values and restore them
expand variables found in values (instead of ending up with $FOO as a string)
easy way to edit the environment variables

Despite its deficiencies, my approach to the problem has a few benefits that I would like for us to consider adding to the excellent foundation established here in this pull request. Namely:

using the already well-established “dotenv” file convention
supporting Virtualfish’s Projects plugin

The `.env` convention

My team has found django-dotenv to be an excellent tool for setting environment varibles on a per-project basis, whereby key=value variables inside ~/projects/our-project/.env are automatically discovered and utilized by our Django project.

This convention is by no means relegated to Django or Python and is a very popular way of handling localized project-specific environment variables: https://encrypted.google.com/search?q=dotenv

Ergo, my suggestion would be to emulate what we already do in other places in Virtualfish, using .env as the default but allowing for the file name to be configurable:

if not set -q VIRTUALFISH_ENVIRONMENT_FILE
    set -g VIRTUALFISH_ENVIRONMENT_FILE .env
end

The key goal and benefit here is having a single environment variable file that can be used not only by Virtualfish, but also by other processes that can read the same standardized file name+format.

Projects plugin support

Related to the .env file convention is the fact that most supporting applications expect it to be in the project root — not inside in the virtual environment. My suggestion would be logic that assumes the project name is the same as the virtual environment name, looks first in the corresponding project root, and if no .env file is found there, then proceeds to look inside the virtual environment. Perhaps something like:

# Get virtualenv name and see if a corresponding project `.env` exists
set -l basename (basename $VIRTUAL_ENV)
if [ -f $PROJECT_HOME/$basename/$VIRTUALFISH_ENVIRONMENT_FILE ]
    set -l env_file_path $PROJECT_HOME/$basename/$VIRTUALFISH_ENVIRONMENT_FILE
else
    set -l env_file_path $VIRTUAL_ENV/$VIRTUALFISH_ENVIRONMENT_FILE
end

if [ -f $env_file_path ]
    [ ... logic that reads and sets the environment variables ... ]

A have a few other thoughts regarding some of the nuts and bolts of the implementation, but I first wanted to discuss these two higher-level topics, both of which I think would be very useful additions to what is shaping up to be an excellent new plugin for Virtualfish. 😃

What do you think?

zsommers · 2017-05-05T19:01:54Z

You make some great points, Justin. I'm a fan of .env files generally and I think it makes sense to keep that sort of project configuration in the project root for the sake of explicitness. I'll take a crack at those changes over this weekend, hopefully.

I'm eager to hear your thoughts about the nuts and bolts. I'm pretty new to Fish, so any advice is welcome.

Thanks!

justinmayer · 2017-05-06T18:28:05Z

I never would have guessed that you are new to Fish. This is some really great work. Bravo!

I'll add some comments on the implementation, all of which are quite minor. 😄

justinmayer

Made some very minor comments. Let me know what you think.

justinmayer · 2017-05-06T18:35:50Z

virtualfish/environment.fish

@@ -0,0 +1,56 @@
+function __vfext_environment_activate --on-event virtualenv_did_activate
+    if test -f $VIRTUAL_ENV/virtualfish-environment
+        for line in (cat $VIRTUAL_ENV/virtualfish-environment)


I don't think there's anything wrong with getting the file contents via cat, but Fish actually has a read command built-in. Here's the method I used:

function __vfext_environment_activate --on-event virtualenv_did_activate if test -r $env_file_path while read line; [ ... logic that sets the environment variables ... ] ; end < $env_file_path end end

Again, I haven't done any performance profiling to compare how the two might differ in speed, but at least wanted to mention this method in case you decide it has merit.

I was a little on the fence about this just because this version reads a little more clearly to me from a code comprehension standpoint.

for line in (foo) # Do work end

However, I also like using built-ins and the prevailing language constructs. Based on what you said and what I read here, the method you recommend is the way to go.

justinmayer · 2017-05-06T18:37:51Z

virtualfish/environment.fish

+
+            # Preserve existing env var with shared name
+            if set -q $key
+                set -xg __VF_ENVIRONMENT_OLD_VALUE_$key $$key


You use set -gx [...] in some places and set -xg [...] in others. Perhaps standardize on set -gx [...] for consistency? (I told you my comments would be minor!) 😉

Great call - consistency is important!

justinmayer · 2017-05-06T18:40:07Z

virtualfish/environment.fish

+
+function __vfext_environment_deactivate --on-event virtualenv_will_deactivate
+    if test -f $VIRTUAL_ENV/virtualfish-environment
+        for line in (cat $VIRTUAL_ENV/virtualfish-environment)


Same comment here with regards to perhaps using Fish's read built-in command. Also, as above, it might be better to use test -r here, which ensures the file exists and is readable.

I'm definitely onboard with -r 😄

justinmayer · 2017-05-06T18:43:18Z

virtualfish/environment.fish

+end
+
+function __vf_environment --description "Edit the environment variables for the active virtual environment"
+    if set -q 'VIRTUAL_ENV'


I'm not sure VIRTUAL_ENV neeeds to be in quotation marks here, but I'm not sure it's doing any harm, either. 😄

I tested this and you're totally right - no quotes needed.

Add configurable filename Add support function to find correct env file path Update activate and deactivate with support function Update edit env file with support function and to actually remove deleted vars from current env Use `read` instead of `cat` Standardize `set -gx`

zsommers · 2017-05-08T18:43:38Z

@justinmayer I made the updates you suggested. I'm interested in your feedback on my implementation of the Project support. I'm not sure dirtying the environment with an extra variable was the best way to handle it, but the logic for finding the correct environment file path was messy enough that I didn't want to replicate it everywhere it was needed. Thanks!

justinmayer · 2017-05-10T16:02:58Z

Hey Zach. Thanks for the follow-up enhancements. I agree that trying to keep the environment clean is a worthy goal, while recognizing that your supporting function to return the correct environment file path helps to keep things DRY.

Instead of setting a global variable, an alternate solution might be for the function to "return" the environment file path via echo. (I've seen this done before in other places in Fish-land.) I'll add some comments to show what I'm talking about. Following is an alternate version I whipped up, along with some other minor suggested changes: https://dpaste.de/tnEF

Let me know what you think. 😄

justinmayer

Made some comments here to try to elaborate on my suggested changes. If my feedback here is confusing in any way (and I fear it might be), please don't hesitate to let me know! 😅

justinmayer · 2017-05-10T16:07:29Z

virtualfish/environment.fish

+    else
+        set -g VIRTUALFISH_ENVIRONMENT_FILE_PATH $vf_path
+    end
+end


We can probably use echo instead of setting a global variable. I also realized the basename call doesn't need to be made if $PROJECT_HOME is not defined, so that should probably be moved further below. I also (very slightly) simplified the environment file path determination logic:

function __vfsupport_env_file_path --description "Return full path to environment file" set -l vf_env_file_path $VIRTUAL_ENV/$VIRTUALFISH_ENVIRONMENT_FILE # Check if Projects plugin is used if set -q PROJECT_HOME set -l vf_name (basename $VIRTUAL_ENV) set -l proj_env $PROJECT_HOME/$vf_name/$VIRTUALFISH_ENVIRONMENT_FILE if test -r $proj_env echo $proj_env else echo $vf_env_file_path end else echo $vf_env_file_path end end

I like your echo suggestion a lot - that does exactly what I was searching for. The reason I used more complicated logic when the Projects plugin is present was for creating a new env file. Here's how my thinking goes:

If we aren't using Projects, place the .env file in the virtualenv directory

If we are using Projects and there is a .env in the project directory, use that file

If we are using Projects and there is not a .env file in the project directory and there is and .env file in the virtualenv directory use that.

(This is why it got a little more complicated) If we are using Projects and there isn't a .env file in either the project or virtualenv directories, create a new file in the project directory.
Case 4 is why I have the logic like this:

if set -q PROJECT_HOME if not test -r $proj_env and test -r $vf_env # Use the virtualenv only when that's been established with a file there else # Prefer project directory end else # Not using Projects end

I think adding some comments and perhaps reordering the logic might make it a bit more clear.

Ah, I hadn't considered that the logic was taking environment file creation into account. Now your logic makes more sense. I agree some comments might help clarify.

It wasn't until I was working on the environment command that I realized that was an issue, just because it opens the file in the $EDITOR regardless of whether it exists. Kind of an edge case, but it caught me. 😄

justinmayer · 2017-05-10T16:09:37Z

virtualfish/environment.fish

-        for line in (cat $VIRTUAL_ENV/virtualfish-environment)
+    __vfsupport_set_env_file_path
+
+    if test -r $VIRTUALFISH_ENVIRONMENT_FILE_PATH


This section would become:

set -l env_file_path (__vfsupport_env_file_path) if test -r $env_file_path

justinmayer · 2017-05-10T16:10:05Z

virtualfish/environment.fish

            end

            # Eval to allow for expanding variables, e.g. PATH=$PATH foo
            set -gx $key (eval echo $value)
-        end
+        end < $VIRTUALFISH_ENVIRONMENT_FILE_PATH


end < $env_file_path

justinmayer · 2017-05-10T16:10:58Z

virtualfish/environment.fish

-    if set -q 'VIRTUAL_ENV'
-        eval $EDITOR $VIRTUAL_ENV/virtualfish-environment
+    if set -q VIRTUAL_ENV
+        __vfsupport_set_env_file_path


set -l env_file_path (__vfsupport_env_file_path)

justinmayer · 2017-05-10T16:11:09Z

virtualfish/environment.fish

        # Deactivate before applying new env vars to avoid stomping stashed values
        __vfext_environment_deactivate
+        eval $EDITOR $VIRTUALFISH_ENVIRONMENT_FILE_PATH


eval $EDITOR $env_file_path

Add comments for additional clarity Reorder some logic for legibility

justinmayer · 2017-05-11T14:28:37Z

Latest code changes look good to me, Zach. Perhaps update the docs to match the revised default .env environment file name?

Once the docs have been updated, I think we are good to merge.

zsommers · 2017-05-16T23:22:13Z

Alright, @justinmayer, sorry for the delay! I've updated the docs and I hope they're pretty clear. Let me know what you think. Thanks!

justinmayer

Looks great to me. Let's get this merged! 👍

zsommers · 2017-05-17T01:36:22Z

Sounds like a plan! Does everything look shipshape to you, @adambrenecki ?

justinmayer · 2017-05-21T18:19:12Z

Hey @adambrenecki. Just checking in. Any thoughts on this PR?

daisylb · 2017-06-11T03:22:16Z

Sorry, everyone! I've been flat out at Uni—and I still am :(

I've had a quick flick through the docs and I'm happy with them and the design of the feature, so I'm going to trust @justinmayer's judgement w.r.t. reviewing the actual code, and blindly merge this :)

zsommers · 2017-06-13T15:47:53Z

Thanks @adambrenecki ! Good luck at Uni and let me know if you come across any issues with this. Thanks for all your help on this, @justinmayer

Vendors plugin, which automatically sets/restores environment variables upon virtual environment activation/deactivation. For more detail, see: justinmayer/virtualfish#119

justinmayer · 2017-06-13T19:21:45Z

Thanks, @adambrenecki. The good news is that if you ever come back to the code later and decide it could be improved, it's totally mutable! 😉

@zsommers: Thanks again for the contribution. Excited to have it merged.

Vendors plugin, which automatically sets/restores environment variables upon virtual environment activation/deactivation. For more detail, see: justinmayer/virtualfish#119

Add environment plugin

48213dc

Add environment plugin and documentation

Activate newly edited env vars

d441640

Allow for empty lines and comments

44bc22f

Add ability to parse empty lines and comments in the `virtualfish-environment` file Added description of `virtualfish-environment` file format to docs

justinmayer reviewed May 6, 2017

View reviewed changes

justinmayer reviewed May 10, 2017

View reviewed changes

Convert env var value passing to echo

d18e966

Add comments for additional clarity Reorder some logic for legibility

zsommers and others added 2 commits May 16, 2017 16:15

Update docs for confgurable file name

050d438

Fix internal link

d0818f3

justinmayer approved these changes May 17, 2017

View reviewed changes

daisylb merged commit 5c5203c into justinmayer:master Jun 11, 2017

justinmayer mentioned this pull request Mar 29, 2019

Added environment variables plugin to automatically set/unset envvars when activating/deactivating a virtualenv #39

Closed

Add environment plugin #119

Add environment plugin #119

Conversation

zsommers commented Apr 18, 2017

daisylb commented Apr 19, 2017

zsommers commented Apr 19, 2017

justinmayer commented Apr 19, 2017

daisylb commented Apr 22, 2017

zsommers commented Apr 22, 2017

zsommers commented May 4, 2017

justinmayer commented May 4, 2017

zsommers commented May 5, 2017

justinmayer commented May 5, 2017

The .env convention

Projects plugin support

zsommers commented May 5, 2017

justinmayer commented May 6, 2017

justinmayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsommers commented May 8, 2017

justinmayer commented May 10, 2017

justinmayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmayer commented May 11, 2017

zsommers commented May 16, 2017

justinmayer left a comment

Choose a reason for hiding this comment

zsommers commented May 17, 2017

justinmayer commented May 21, 2017

daisylb commented Jun 11, 2017

zsommers commented Jun 13, 2017

justinmayer commented Jun 13, 2017

The `.env` convention