MichaelAquilina/zsh-autoswitch-virtualenv

Environment pollution

rnc opened this issue · 10 comments

rnc commented

( Fedora 30 ; ZSH 5.7.1 ; Manually installed from git )

This is not an issue as such, more of a discussion :-)

Currently colours such as RED etc are defined within the file. If this is sourced then these end up within the shell environment.

I am wondering whether it might be better to wrap them inside a function (used by the other functions) to avoid these 'polluting' the parent shell environment?

MichaelAquilina commented

Currently colours such as RED etc are defined within the file. If this is sourced then these end up within the shell environment.

I was actually wondering about this last time but then noticed that my environment doesnt have this issue.

env | grep RED
<no output>

I install this repo from zplug so I wonder if that does something special to prevent globals defined in plugins from entering the user's environment.

I am wondering whether it might be better to wrap them inside a function (used by the other functions) to avoid these 'polluting' the parent shell environment?

Problem with this approach is that you are just moving the problem. Instead of polluting the user's environment variables you are poluting the user's list of available commands.

I tend to prefix my functions that are not meant for general use with _ to make it obvious they are not meant to be used directly and make them unlikely to be tab completed. So maybe that is slightly better than having environment variables.

I would be more interesed in seeing what zplug does to prevent this issue and recommend a similar approach for manual installation instead.

rnc commented

Thats interesting! I wonder how it is doing that. From having a very brief look at zplug there is a lot of code so its not obvious how its loading something as a plugin and what it does with functions / variables.
( and yeah, it is just moving the problem somewhat ).

@MichaelAquilina Question - does zplug load the functions as autoloads initially?

MichaelAquilina commented

I'm afraid I'm not familiar enough with autoloads to know for sure! Are you aware of any good documentation I can use for reference?

How are you loading in this plugin? From what I gather, it seems like you load it in manually. But I thought I'd double check before assuming.

PS: Yeah zplug has an insane codebase 😱

rnc commented

I think that a function is autoloaded if when in a new shell you see e.g.

zmv () {
        # undefined
        builtin autoload -XU
}

I have used http://zsh.sourceforge.net/Doc/Release/Completion-System.html#Autoloaded-files and https://www.refining-linux.org/archives/46-ZSH-Gem-12-Autoloading-functions.html before.

I am loading it with https://github.com/rnc/nicks-shell/blob/master/.zshrc#L190 - very manual setup :-)

I wonder if other frameworks, when they load it, also have the same affect as zplug?

I don't really think its worth changing anything .... its more out of curiosity now! :-)

rnc commented

@MichaelAquilina I think I may have found it! After experimenting with oh-my-zsh as well and seeing that it also doesn't have e.g. RED, NORMAL in the environment I tried experimenting with a clean user. This led me to the fact I was setting ALL_EXPORT ; using

        () {
            setopt local_options no_all_export
            source $NS_PREFIX/zsh-autoswitch-virtualenv/autoswitch_virtualenv.plugin.zsh
        }

works around it.

rnc commented

+1, yes exactly. Its been there for ages and I'd, frankly, forgotten about it :-(

MichaelAquilina commented

I wasn't even aware the option existed so I learnt something new today :) Are you happy with closing this issue or do you think there is more to discuss?

rnc commented

Yeah its fine to close - unless you wanted me to add a sentence to the readme for the manual install about this? Something like

Its recommended not to have `ALL_EXPORT` enabled so that the variables defined in the plugin do not appear in your environment.
MichaelAquilina commented

I think considering that ALL_EXPORT is turned off by default and enabling it gives the expected behaviour above - we shouldnt need to specify it. But at least I am now aware of it in case anyone else asks a similar question in the future.

Closing the issue as I'm happy with the status as is. Thanks for the help @rnc and good find :)