Repository: xonsh/xonsh
Pull request: EnvPath methods (append, remove, add, insert) prepare the path
Closes #2468

Before:
```xsh
$PATH.append(p"~/node_modules/.bin")
$PATH.remove(p"~/node_modules/.bin")
# ValueError

$PATH.append("~/node_modules/.bin")
$PATH.remove(p"~/node_modules/.bin")
# ValueError

$PATH.append($HOME+"/node_modules/.bin")
$PATH.remove(p"~/node_modules/.bin")
# ValueError

$PATH.prepend("/path")
# 'EnvPath' object has no attribute 'prepend'
```

After:

All cases are working well.


## For community
⬇️  **Please click the 👍 reaction instead of leaving a `+1` or 👍  comment**


Problem statement:
Can't remove Path objects from $PATH
If you add a `Path` object to `$PATH`, it is coerced to a string. However, the same coercion doesn't happen with `.remove()`

For example, this fails:

```
🐚 $PATH.append(p"~/node_modules/.bin")
🐚 $PATH.remove(p"~/node_modules/.bin")
ValueError
```

(The work-around is to cast it to a `str` in the second line.)

## For community
⬇️  **Please click the 👍 reaction instead of leaving a `+1` or 👍  comment**

Hints from issue discussion:
If anybody wants to add the path temporarily there is `swap`:
```python
print($PATH)
with ${...}.swap(PATH=$PATH+[p"~/node_modules/.bin"]):
      print($PATH)
print($PATH)

# ['/bin', '/usr/games', '/usr/local/games', '/snap/bin']
# ['/bin', '/usr/games', '/usr/local/games', '/snap/bin', PosixPath('/home/user/node_modules/.bin')]
# ['/bin', '/usr/games', '/usr/local/games', '/snap/bin']

```
Oh this is actually a bit more confusing than it looks. `$PATH.append(p"/usr/foo")` actually doesn't coerce the path to a string. It's coerced to a string when `$PATH` is inherited to subprocesses, but other operations are a bit more subtle.

A simple `$PATH` "command" in xonsh doesn't print its `repr()`, or its `str()`, but uses xonsh's pretty printing mechanism. `EnvPath._repr_pretty_` iterates over `self`:
https://github.com/xonsh/xonsh/blob/17a43bd073323ed60395413590bce4b2bc62c8e1/xonsh/tools.py#L248-L254

That calls `EnvPath.__getitem__()`: https://github.com/xonsh/xonsh/blob/17a43bd073323ed60395413590bce4b2bc62c8e1/xonsh/tools.py#L214-L219

Which calls `_expand_path()`, which eventually calls (on Unix) `os.path.expanduser()`. *That* transparently takes `pathlib.Path`s but returns strings!

All of this means that `$PATH` will look like a list of strings, and `$PATH[-1]` will return a string, but the actual values aren't:
```xonsh
@ $PATH = ["/usr/foo", p"/usr/bar"]
@ $PATH
EnvPath(
['/usr/foo', '/usr/bar']
)
@ print(str($PATH))
['/usr/foo', PosixPath('/usr/bar')]
```

Given that… I'm not sure what the right solution is here. `EnvPath` could override basically every `MutableSequence` method (like `.append` and `.remove`) to normalize the arguments, or `EnvPath` could normalize its contents on `__setitem__` and `insert` — but in that case, normalize them to what? `bytes` and `pathlib.Path` both would have compelling arguments.

Commit message: [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Patch to review:
diff --git a/news/path_imp.rst b/news/path_imp.rst
new file mode 100644
index 0000000000..6abbb59149
--- /dev/null
+++ b/news/path_imp.rst
@@ -0,0 +1,23 @@
+**Added:**
+
+* EnvPath shows warning if path that added does not exists.
+
+**Changed:**
+
+* EnvPath methods (append, remove, add, insert) prepare the path before add.
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* <news item>
+
+**Security:**
+
+* <news item>
diff --git a/tests/test_tools.py b/tests/test_tools.py
index f37edc0e6f..d96c9e384d 100644
--- a/tests/test_tools.py
+++ b/tests/test_tools.py
@@ -1014,6 +1014,46 @@ def test_env_path_add_pathlib():
     ]
 
 
+def test_env_path_append_remove_pathlib_path():
+    path = EnvPath()
+
+    # Append-remove
+    path.append(os.sep.join(["home", "dino"]))
+    path.remove(os.sep.join(["home", "dino"]))
+
+    path.append(os.sep.join(["~", "dino"]))
+    path.remove(pathlib.Path(os.sep.join(["~", "dino"])))
+
+    path.append(pathlib.Path(os.sep.join(["~", "dino"])))
+    path.remove(pathlib.Path(os.sep.join(["~", "dino"])))
+
+    path.append(pathlib.Path(os.sep.join(["~", "dino"])))
+    path.remove(os.sep.join(["~", "dino"]))
+
+    path.append(
+        pathlib.Path(os.sep.join([str(pathlib.Path("~").expanduser()), "dino"]))
+    )
+    path.remove(os.sep.join(["~", "dino"]))
+
+    path.append(pathlib.Path(os.sep.join(["~", "dino"])))
+    path.remove(os.sep.join([str(pathlib.Path("~").expanduser()), "dino"]))
+
+    # Insert-remove
+    path.insert(0, os.sep.join(["home", "dino"]))
+    path.remove(os.sep.join(["home", "dino"]))
+
+    path.insert(0, os.sep.join(["~", "dino"]))
+    path.remove(pathlib.Path(os.sep.join(["~", "dino"])))
+
+    path.insert(0, pathlib.Path(os.sep.join(["~", "dino"])))
+    path.remove(pathlib.Path(os.sep.join(["~", "dino"])))
+
+    path.insert(0, pathlib.Path(os.sep.join(["~", "dino"])))
+    path.remove(os.sep.join(["~", "dino"]))
+
+    assert path == []
+
+
 # helper
 def expand(path):
     return os.path.expanduser(os.path.expandvars(path))
diff --git a/xonsh/tools.py b/xonsh/tools.py
index 5a06156147..c390e95ac5 100644
--- a/xonsh/tools.py
+++ b/xonsh/tools.py
@@ -228,8 +228,28 @@ def __len__(self):
     def __delitem__(self, key):
         self._l.__delitem__(key)
 
+    @staticmethod
+    def _prepare_path(p):
+        return str(expand_path(p))
+
+    @staticmethod
+    def _check_path(p):
+        if not pathlib.Path(p).exists():
+            print(f"EnvPath warning: path {repr(p)} does not exists.", file=sys.stderr)
+
     def insert(self, index, value):
-        self._l.insert(index, value)
+        self._l.insert(index, self._prepare_path(value))
+        self._check_path(value)
+
+    def append(self, value):
+        self._l.append(self._prepare_path(value))
+        self._check_path(value)
+
+    def remove(self, value):
+        try:
+            self._l.remove(self._prepare_path(value))
+        except ValueError:
+            print(f"EnvPath warning: path {repr(value)} not found.", file=sys.stderr)
 
     @property
     def paths(self):
@@ -292,13 +312,14 @@ def add(self, data, front=False, replace=False):
         None
 
         """
-        data = str(expand_path(data))
+        data = self._prepare_path(data)
         if data not in self._l:
             self._l.insert(0 if front else len(self._l), data)
         elif replace:
             # https://stackoverflow.com/a/25251306/1621381
             self._l = list(filter(lambda x: x != data, self._l))
             self._l.insert(0 if front else len(self._l), data)
+        self._check_path(data)
 
 
 class FlexibleFormatter(string.Formatter):


Code review comments:
diff_hunk: @@ -228,8 +228,28 @@ def __len__(self):
     def __delitem__(self, key):
         self._l.__delitem__(key)
 
+    @staticmethod
+    def _prepare_path(p):
+        return str(expand_path(p))
+
+    @staticmethod
+    def _check_path(p):
+        if not pathlib.Path(p).exists():
+            print(f"EnvPath warning: path {repr(p)} does not exists.", file=sys.stderr)
path: xonsh/tools.py
text: @user1:
It is not needed to check by default. Many of the times, the dirs in $PATH would have actually been deleted or yet to be created. 

@author:
@user1 removed