Fixing old sh(...) non-documented method to define special args defaults

[Lucas-C]

This pull request is an implementation of the changes discussed in #269.

I ended up defining a context manager as its seemed to me the safest and most elegant solution.

Note that this commit removes the old, non-documented, usage to set defaults for all sh commands, and its only related test: https://github.com/amoffat/sh/blob/7a4e296/test.py#L1502

The reason beeing that it created a very confusing edge case:

import sh, sys
sh = sh(_out=sys.stdout)
from sh import echo  # this command still has _out=None as default

I tried many things to work around this issue, but in the end I don't think we can safely allow the from sh import ... mecanism AND provide a way to reassign sh. It is fundamentally incompatible, due to Python sys.modules caching system.

As for the documentation, I couldn't find the source anywhere than on http://amoffat.github.io/sh/, so I generated a patch:
https://gist.github.com/Lucas-C/4f466b14061c5fbfbbd8

[amoffat]

Cool, I'll try to find time this week to review. The one thing that stands out, and you'll want to verify this for yourself, is I believe this yield https://github.com/amoffat/sh/pull/271/files#diff-2c2fab1457aa782cf8f2f34ba9ae0975R2299 needs a try/finally, otherwise the environment won't be restored in the case of an exception that bubbles up to the user's code.

[Lucas-C]

Good catch, I'll fix that asap.

Also, the test_done_callback test seems to be failing in Travis due to a timing issue:
https://travis-ci.org/amoffat/sh/jobs/77056729
Have you seen this kind of test failure before ?

[amoffat]

Yeah don't worry about it. Those Travis machines have very unpredictable timing.

[amoffat]

The semantics of the context manager are confusing. When a context manager is instantiated, typically it is used to use it explicitly to manage state. The problem I have with this context manager is there is no explicit state:

with sh.defaults(_out=out):
    from sh import echo

sh.defaults is implicitly changing a global setting on sh, but that is not clear to someone reading the code, because context managers typically provide a handle to manage state:

with sh.defaults(_out=out) as sh2:
    from sh2 import echo # new echo
    from sh import echo # old echo

If the state is implicit, we still run into the old problem I am trying to avoid of globally changing the behavior of sh for another module:

with sh.defaults(_out=out):
    anothermodule.some_function()

Where anothermodule.some_function has unexpected behavior of the sh module.

I want to continue in the direction of explicit state management, so I cannot merge this PR at the moment.

[Lucas-C]

Strange, I have this strange behaviour where some tox tests fail, but when I run them individually with

tox FunctionalTests.test_defaults_with_global_module

Or

python$version test.py FunctionalTests.test_defaults_with_global_module

They pass

Any idea why ??

[amoffat]

I haven't tried it yet, but I'm going to guess that there is state bleeding between the tests. I would look into removing the module from sys.modules in the tearDown of the test class, and see if that fixes it. And there doesn't seem to be an existing tearDown, so you will have to add one.

[Lucas-C]

Despite the Travis checks failing (because of Python 3+ installation issues I think),
this commit looks good to me.
What do you think ?

[amoffat]

@Lucas-C lots of tests, that is great to see :) It will take me awhile to review this..there's quite a bit of code and magic going on, but thanks for the effort you put into the PR. After I finish reviews, I can see this landing in the 1.2 branch

[Lucas-C]

I'm glad to read this !
And sure, take your time :)

[amoffat]

What was the reason for removing the __env stuff?

[Lucas-C]

I did not remove the __env, just the __setattr__ magic method.
The reason is that I did not find any use case for it, and all the tests passed ok without it.
So I just removed it to simplify the code.

[amoffat]

Apologies for being so slow on this.

I actually have some really bad news about this PR. The last few days, I was tinkering with it (just to understand it better), and I noticed the tests fail on python3.3 and python3.4 (but work in 3.2, 3.1 and 3). It turns out that in python3.3 and later, the internal import code requires any module that you load with a fancy importer (like we've done here) be put into sys.modules.

However, as your docstring on load_module states, we deliberately don't put the module in sys.modules, the reason being that if it's cached, this no longer works:

import sh

sh1 = sh(_out="/dev/null")
from sh1 import ls
print(ls()) # shouldn't print

sh1 = sh()
from sh1 import ls
print(ls()) # SHOULD print, but doesn't!

sh1 has been cached in sys.modules, and so the second from sh1 import ls re-uses that existing module, with the old baked (_out="/dev/null") params.

There really doesn't seem to be a way around this. Python 3.3 and up require the module in sys.modules, and if it's in there, tests fail and behavior breaks. I think best-case scenario is we remove the from / import and just go with using attribute lookups, like sh1.ls() instead. This is a real bummer because of the amount of work that went into the discussion and implementation. I'm open to any ideas.

[amoffat]

Actually, scratch that last message. I think I found a way around the limitation, but it's horrible and I feel bad for writing it. But it works!

It allows us to put our module in sys.modules, and then preemptively remove it if another context would try to re-use it

[Lucas-C]

Glad you found the time to get back to this PR ! ^^
OMG, yes, this is very hacky. I wish we wouldn't need to do such horrible stack frame manipulation...
So, this will be released in 1.2 ?

[amoffat]

It will, yes. I am aiming for some time next week if I can make it through all the issues and PRs by then