Repository: amoffat/sh (python)

Issue:
wrong type for _env leads to confusing error
When the type of **_env** is a **set** instead of a **dict** (a common error when using a coma instead of a colon), the error raised suggests the command itself failed and does not hint that the type of the **_env** argument is incorrect.

```python
>>> sh.env(_env={'FOO': 'bar'})
FOO=bar

>>> sh.env(_env={'FOO', 'bar'})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/venv/lib/python3.6/site-packages/sh.py", line 1427, in __call__
    return RunningCommand(cmd, call_args, stdin, stdout, stderr)
  File "/home/venv/lib/python3.6/site-packages/sh.py", line 774, in __init__
    self.wait()
  File "/home/venv/lib/python3.6/site-packages/sh.py", line 792, in wait
    self.handle_command_exit_code(exit_code)
  File "/home/venv/lib/python3.6/site-packages/sh.py", line 815, in handle_command_exit_code
    raise exc
sh.ErrorReturnCode_255: 

  RAN: /usr/bin/env

  STDOUT:


  STDERR:
```

Pull request:
Add a validator for env. Fixes #460
I had to disable old Python versions in Travis to get the test run to pass. These old versions can no longer be installed.

Patch:
diff --git a/README.rst b/README.rst
index 1172b26..b4ad174 100644
--- a/README.rst
+++ b/README.rst
@@ -20,7 +20,7 @@
 |
 
 sh is a full-fledged subprocess replacement for Python 2.6 - 3.6, PyPy and PyPy3
-that allows you to call *any* program as if it were a function:
+that allows you to call any program as if it were a function:
 
 .. code:: python
 
diff --git a/sh.py b/sh.py
index e277949..b14de5c 100644
--- a/sh.py
+++ b/sh.py
@@ -87,11 +87,6 @@ else:
     from io import BytesIO as iocStringIO
     from Queue import Queue, Empty
 
-try:
-    from shlex import quote as shlex_quote # here from 3.3 onward
-except ImportError:
-    from pipes import quote as shlex_quote # undocumented before 2.7
-
 IS_OSX = platform.system() == "Darwin"
 THIS_DIR = os.path.dirname(os.path.realpath(__file__))
 SH_LOGGER_NAME = __name__
@@ -697,7 +692,7 @@ class RunningCommand(object):
         # arguments are the encoding we pass into _encoding, which falls back to
         # the system's encoding
         enc = call_args["encoding"]
-        self.ran = " ".join([shlex_quote(arg.decode(enc, "ignore")) for arg in cmd])
+        self.ran = " ".join([arg.decode(enc, "ignore") for arg in cmd])
 
         self.call_args = call_args
         self.cmd = cmd
@@ -1056,6 +1051,28 @@ def bufsize_validator(kwargs):
     return invalid
 
 
+def env_validator(kwargs):
+    """ a validator to check that env is a dictionary and that all environment variable 
+    keys and values are strings. Otherwise, we would exit with a confusing exit code 255. """
+    invalid = []
+
+    env = kwargs.get("env", None)
+    if env is None:
+        return invalid
+
+    if not isinstance(env, dict):
+        invalid.append((("env"), "env must be a dict. Got {!r}".format(env)))
+        return invalid
+
+    for k, v in kwargs["env"].items():
+        if not isinstance(k, str):
+            invalid.append((("env"), "env key {!r} must be a str".format(k)))
+        if not isinstance(v, str):
+            invalid.append((("env"), "value {!r} of env key {!r} must be a str".format(v, k)))
+
+    return invalid
+
+
 class Command(object):
     """ represents an un-run system program, like "ls" or "cd".  because it
     represents the program itself (and not a running instance of it), it should
@@ -1193,6 +1210,7 @@ class Command(object):
         (("close_fds", "pass_fds"), "Passing `pass_fds` forces `close_fds` to be True"),
         tty_in_validator,
         bufsize_validator,
+        env_validator,
     )
 
 
@@ -1858,7 +1876,7 @@ class OProc(object):
         session_pipe_read, session_pipe_write = os.pipe()
         exc_pipe_read, exc_pipe_write = os.pipe()
 
-        # this pipe is for synchronizing with the child that the parent has
+        # this pipe is for synchronzing with the child that the parent has
         # closed its in/out/err fds.  this is a bug on OSX (but not linux),
         # where we can lose output sometimes, due to a race, if we do
         # os.close(self._stdout_write_fd) in the parent after the child starts
@@ -1966,7 +1984,7 @@ class OProc(object):
                     close_fds = True
 
                 if close_fds:
-                    pass_fds = set((0, 1, 2))
+                    pass_fds = set((0, 1, 2, exc_pipe_write))
                     pass_fds.update(ca["pass_fds"])
 
                     # don't inherit file descriptors
@@ -2273,13 +2291,13 @@ class OProc(object):
 
     def get_pgid(self):
         """ return the CURRENT group id of the process. this differs from
-        self.pgid in that this reflects the current state of the process, where
+        self.pgid in that this refects the current state of the process, where
         self.pgid is the group id at launch """
         return os.getpgid(self.pid)
 
     def get_sid(self):
         """ return the CURRENT session id of the process. this differs from
-        self.sid in that this reflects the current state of the process, where
+        self.sid in that this refects the current state of the process, where
         self.sid is the session id at launch """
         return os.getsid(self.pid)
 
@@ -3449,7 +3467,7 @@ class ModuleImporterFromVariables(object):
             derp = sh()
             from derp import ls
 
-        here, mod_fullname will be "derp".  keep that in mind as we go through
+        here, mod_fullname will be "derp".  keep that in mind as we go throug
         the rest of this function """
 
         parent_frame = inspect.currentframe().f_back
@@ -3480,7 +3498,7 @@ class ModuleImporterFromVariables(object):
         module = fetch_module_from_frame(mod_fullname, parent_frame)
 
         # we HAVE to include the module in sys.modules, per the import PEP.
-        # older versions of python were more lenient about this being set, but
+        # older verions of python were more lenient about this being set, but
         # not in >= python3.3, unfortunately.  this requirement necessitates the
         # ugly code in SelfWrapper.__call__
         sys.modules[mod_fullname] = module


Test patch:
diff --git a/test.py b/test.py
index 9872eac..95d4934 100644
--- a/test.py
+++ b/test.py
@@ -647,6 +647,17 @@ print(os.listdir("/dev/fd"))
         self.assertRaises(TypeError, ls, _iter=True, _piped=True)
 
 
+    def test_invalid_env(self):
+        from sh import ls
+
+        exc = TypeError
+        if IS_PY2 and MINOR_VER == 6:
+            exc = ValueError
+
+        self.assertRaises(exc, ls, _env="XXX")
+        self.assertRaises(exc, ls, _env={"foo": 123})
+        self.assertRaises(exc, ls, _env={123: "bar"})
+
     def test_exception(self):
         from sh import ErrorReturnCode_2