Merge pull request #3748 from bobhy/environment_fixes

Environment fixes

news/environment_fixes.rst

@@ -0,0 +1,30 @@
+**Added:**
+
+* <news item>
+
+**Changed:**
+
+* xonsh/environ.py: new rule: for "registered" environment variables (in ``DEFAULT_VARS`` or via ``env.register()``), 
+  if default is set to ``DefaultNotGiven``, then variable has no default and raises ``KeyError`` if it is not 
+  actually defined in environment.  Likewise, ``"var" in __xonsh__.env`` will return False.
+* Changed defaults for ANSICON, TERM and VIRTUAL_ENV to ``DefaultNotGiven``, so code can rationally test whether
+  the expected external program has defined these variables.  No need to do this for variables that xonsh
+  itself defines.
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* Fixed #3703 and #3739, recent code change made it impossible to tell whether a (registered) environment variable
+  was missing from environment or present and set to its registered default value. The test for ANSICON was
+  failing due to this.
+
+**Security:**
+
+* <news item>

tests/test_environ.py

@@ -480,16 +480,22 @@ def test_env_iterate_rawkeys():
     assert saw_regex
 
 
-@pytest.mark.parametrize(
-    "do_register,doc_default",
-    [(True, "abc"), (True, DefaultNotGiven), (False, "abc"), (False, DefaultNotGiven)],
-)
-def test_env_get_docs(do_register, doc_default):
-    """Verify get_docs.doc_default handles both known and unknown environment variables."""
-    env = Env()
-    if do_register:
-        env.register("MY_SPECIAL_VAR", type="str", default="", doc_default=doc_default)
+def test_env_get_defaults():
+    """Verify the rather complex rules for env.get("<envvar>",default) value when envvar is not defined.
+    """
 
-    docs = env.get_docs("MY_SPECIAL_VAR")
+    env = Env(TEST1=0)
+    env.register("TEST_REG", default="abc")
+    env.register("TEST_REG_DNG", default=DefaultNotGiven)
 
-    assert isinstance(docs.doc_default, str)
+    # var is defined, registered is don't-care => value is defined value
+    assert env.get("TEST1", 22) == 0
+    # var not defined, not registered => value is immediate default
+    assert env.get("TEST2", 22) == 22
+    assert "TEST2" not in env
+    # var not defined, is registered, reg default is not sentinel => value is *registered* default
+    assert env.get("TEST_REG", 22) == "abc"
+    assert "TEST_REG" in env
+    # var not defined, is registered, reg default is sentinel => value is *immediate* default
+    assert env.get("TEST_REG_DNG", 22) == 22
+    assert "TEST_REG_DNG" not in env

tests/tools.py

@@ -109,7 +109,6 @@ class DummyEnv(MutableMapping):
     DEFAULTS = {
         "XONSH_DEBUG": 1,
         "XONSH_COLOR_STYLE": "default",
-        "VIRTUAL_ENV": "",
     }
 
     def __init__(self, *args, **kwargs):

xonsh/environ.py

@@ -649,7 +649,9 @@ convert : func
 detype : func
     Function to convert variable from its type to a string representation.
 default
-    Default value for variable.
+    Default value for variable. If set to DefaultNotGiven, raise KeyError
+    instead of returning this default value.  Used for env vars defined
+    outside of Xonsh.
 doc : str
    The environment variable docstring.
 doc_configurable : bool, optional
@@ -686,7 +688,7 @@ def DEFAULT_VARS():
             is_string,
             ensure_string,
             ensure_string,
-            "",
+            DefaultNotGiven,
             "This is used on Windows to set the title, if available.",
             doc_configurable=False,
         ),
@@ -1285,9 +1287,10 @@ def DEFAULT_VARS():
             is_string,
             ensure_string,
             ensure_string,
-            "",
+            DefaultNotGiven,
             "TERM is sometimes set by the terminal emulator. This is used (when "
-            "valid) to determine whether or not to set the title. Users shouldn't "
+            "valid) to determine whether the terminal emulator can support "
+            "the selected shell, or whether or not to set the title. Users shouldn't "
             "need to set this themselves. Note that this variable should be set as "
             "early as possible in order to ensure it is effective. Here are a few "
             "options:\n\n"
@@ -1394,7 +1397,7 @@ def DEFAULT_VARS():
             is_string,
             ensure_string,
             ensure_string,
-            "",
+            DefaultNotGiven,
             "Path to the currently active Python environment.",
             doc_configurable=False,
         ),
@@ -1860,7 +1863,7 @@ class Env(cabc.MutableMapping):
 
     def get_default(self, key, default=None):
         """Gets default for the given key."""
-        if key in self._vars:
+        if key in self._vars and self._vars[key].default is not DefaultNotGiven:
             return self._vars[key].default
         else:
             return default
@@ -1871,7 +1874,12 @@ class Env(cabc.MutableMapping):
         if vd is None:
             vd = Var(default="", doc_default="")
         if vd.doc_default is DefaultNotGiven:
-            dval = pprint.pformat(vd.default)
+            var_default = self._vars.get(key, "<default not set>").default
+            dval = (
+                "not defined"
+                if var_default is DefaultNotGiven
+                else pprint.pformat(var_default)
+            )
             vd = vd._replace(doc_default=dval)
         return vd
 
@@ -1936,7 +1944,7 @@ class Env(cabc.MutableMapping):
             return self
         elif key in self._d:
             val = self._d[key]
-        elif key in self._vars:
+        elif key in self._vars and self._vars[key].default is not DefaultNotGiven:
             val = self.get_default(key)
             if is_callable_default(val):
                 val = val(self)
@@ -1987,16 +1995,25 @@ class Env(cabc.MutableMapping):
         """The environment will look up default values from its own defaults if a
         default is not given here.
         """
-        try:
+        if key in self._d or (
+            key in self._vars and self._vars[key].default is not DefaultNotGiven
+        ):
             return self[key]
-        except KeyError:
+        else:
             return default
 
     def rawkeys(self):
         """An iterator that returns all environment keys in their original form.
         This include string & compiled regular expression keys.
         """
-        yield from (set(self._d) | set(self._vars))
+        yield from (
+            set(self._d)
+            | set(
+                k
+                for k in self._vars.keys()
+                if self._vars[k].default is not DefaultNotGiven
+            )
+        )
 
     def __iter__(self):
         for key in self.rawkeys():
@@ -2004,7 +2021,9 @@ class Env(cabc.MutableMapping):
                 yield key
 
     def __contains__(self, item):
-        return item in self._d or item in self._vars
+        return item in self._d or (
+            item in self._vars and self._vars[item].default is not DefaultNotGiven
+        )
 
     def __len__(self):
         return len(self._d)

xontrib/voxapi.py

@@ -273,8 +273,6 @@ class Vox(collections.abc.Mapping):
         """
         if name is ...:
             env = builtins.__xonsh__.env
-            if not env["VIRTUAL_ENV"]:
-                raise KeyError()
             env_paths = [env["VIRTUAL_ENV"]]
         elif isinstance(name, PathLike):
             env_paths = [fspath(name)]
@@ -336,7 +334,7 @@ class Vox(collections.abc.Mapping):
         Returns None if no environment is active.
         """
         env = builtins.__xonsh__.env
-        if not env["VIRTUAL_ENV"]:
+        if "VIRTUAL_ENV" not in env:
             return
         env_path = env["VIRTUAL_ENV"]
         if env_path.startswith(self.venvdir):
@@ -358,7 +356,7 @@ class Vox(collections.abc.Mapping):
         """
         env = builtins.__xonsh__.env
         ve = self[name]
-        if env["VIRTUAL_ENV"]:
+        if "VIRTUAL_ENV" in env:
             self.deactivate()
 
         type(self).oldvars = {"PATH": list(env["PATH"])}
@@ -374,7 +372,7 @@ class Vox(collections.abc.Mapping):
         Deactivate the active virtual environment. Returns its name.
         """
         env = builtins.__xonsh__.env
-        if not env["VIRTUAL_ENV"]:
+        if "VIRTUAL_ENV" not in env:
             raise NoEnvironmentActive("No environment currently active.")
 
         env_name = self.active()