acatton / python-spm

:muscle: Simple and secure sub processes manager
https://python-spm.readthedocs.org/en/latest/
MIT License
10 stars 3 forks source link

No environment by default (Fix #8) #9

Closed acatton closed 9 years ago

acatton commented 9 years ago

I think #8 was a great idea. I explained why in the documentation in this patch.

However I don't like "update_environment". I would like a better name. But I couldn't find one.

Any suggestion @rockymeza @gavinwahl?

rockymeza commented 9 years ago

Suggestion 1:

Make it a separate kwarg.

from spm import run

run('echo', env={'k': 'v'}, propagate_env=True)

I know right, blegh, but it allows you to keep the ('env', '-') thing if you want? (maybe the environment is too huge to copy?

Suggestion 2:

Offload the environment merging to user:

import os
from spm import run

def merge(*dicts):
    ret = {}
    for dict in dicts:
        ret.update(dict)
    return ret

run('echo', env=merge(os.environ, {'k': 'v'}))

If you want, you can include merge in spm. There's even a great stackoverflow answer on how to write merge_dicts. It mentions the possibility for this:

import os
from spm import run

# valid Python 3.5 code!
run('echo', env={**os.environ, 'k': 'v'})

I'm obviously more a fan of Solution 2:

acatton commented 9 years ago

Okay, after investigating on this. I was leaning towards run('echo', env=merge(os.environ, other_dict)), I started implementing it, but I realized that they're is no way to keep track if the environment has been propagated or not. So this would generate a huge env ... command when trying to cast the command to a string.

So I decided to rename update_env to propagate_env.