Repository: xonsh/xonsh
Pull request: Fix: EnvPath.add() not maintain uniqueness with Path objects on replace
<!--- Thanks for opening a PR on xonsh! Please include a news entry with your PR
to help keep our changelog up to date! There are instructions available here:
https://xon.sh/devguide.html#changelog -->

<!--- If there is specific issue / feature request that this PR is addressing,
please link to the corresponding issue by using the `#issuenumber` syntax.
Thanks again! -->
Fixes #4366

Tested via docker python.
![image](https://user-images.githubusercontent.com/1596188/132269422-df894b81-1c1d-4f80-9db7-512abd253eee.png)

Original credits for the filter out duplicates to source: https://stackoverflow.com/a/25251306/1621381  
Seems to work great when testing.

## For community
⬇️  **Please click the 👍 reaction instead of leaving a `+1` or 👍  comment**



Problem statement:
EnvPath.add() doesn't maintain uniqueness with Path objects
## xonfig

<details>

```
+------------------+----------------------+
| xonsh            | 0.9.27               |
| Git SHA          | 71fe9014             |
| Commit Date      | Jan 29 08:58:58 2021 |
| Python           | 3.9.5                |
| PLY              | 3.11                 |
| have readline    | True                 |
| prompt toolkit   | None                 |
| shell type       | readline             |
| pygments         | 2.9.0                |
| on posix         | True                 |
| on linux         | True                 |
| distro           | ubuntu               |
| on darwin        | False                |
| on windows       | False                |
| on cygwin        | False                |
| on msys2         | False                |
| is superuser     | False                |
| default encoding | utf-8                |
| xonsh encoding   | utf-8                |
| encoding errors  | surrogateescape      |
| on jupyter       | False                |
| jupyter kernel   | None                 |
| xontrib 1        | apt_tabcomplete      |
| xontrib 2        | direnv               |
| xontrib 3        | kitty                |
| xontrib 4        | linuxbrew            |
+------------------+----------------------+
```

</details>

## Expected Behavior
`EnvPath.add(thing)` (e.g. `$PATH.add(thing)`) should not allow multiple copies of `thing` to enter the list, compared as strings.

## Current Behavior (and reproduction steps)
`EnvPath.add("str")` and `EnvPath.add(p"str")` do not detect each other's entries:

```
egnor@ostrich ~ $ $PATH
EnvPath(
['/usr/local/sbin',
 '/usr/local/bin',
 '/usr/sbin',
 '/usr/bin',
 '/sbin',
 '/bin',
 '/usr/games',
 '/usr/local/games',
 '/snap/bin']
)
egnor@ostrich ~ $ $PATH.add("/home/egnor/bin")
egnor@ostrich ~ $ $PATH.add(p"~/bin")
egnor@ostrich ~ $ $PATH
EnvPath(
['/usr/local/sbin',
 '/usr/local/bin',
 '/usr/sbin',
 '/usr/bin',
 '/sbin',
 '/bin',
 '/usr/games',
 '/usr/local/games',
 '/snap/bin',
 '/home/egnor/bin',
 '/home/egnor/bin']
)
```

This is because `EnvPath.add(data)` will put its data (of any type, `Path` or `str`) into the object directly, and `Path("foo") != "foo"`.

My first instinct was to have `EnvPath.add(data)` include `data = str(data)`, but then I realized that `EnvPath.__init__` was explicitly written to allow a mixed bag of `Path` and `str` objects, so I wasn't sure what was desired. `EnvPath` is documented as being a "list of strings" so I would expect it to stringify everything on entry, but maybe there's some concerns with that?

Note that `$PATH.add(p"~/bin")` seems like a very logical and idiomatic thing to put in one's `.xonshrc`, but due to this bug will end up with repeated copies when subshells occur.

## For community
⬇️  **Please click the 👍 reaction instead of leaving a `+1` or 👍  comment**

Hints from issue discussion:
Hi!

> EnvPath.add(thing) (e.g. $PATH.add(thing)) should not allow multiple copies of thing to enter the list, compared as strings.

Why? 
AFAIK the OS doesn't mind and other shells allow this as well. 
The `.add` method is specifically documented not to add duplicates (with
flags controlling precise de-duping semantics), and in general when all the
items are "str" it does not, and that seems to be that method's whole
reason to exist, vs `.append` or whatnot.

The general use case is adding to `$PATH` or similar strings in one's
`.xonshrc` without ending up with an ever expanding path full of dupes due
to subshells that launch each other for various reasons.

This all works just fine but the `Path`/`str` mixture seems to break that.

>

Oh sorry I missed you used add instead of append

On Sat, Jul 17, 2021, 18:59 Daniel Egnor ***@***.***> wrote:

> The `.add` method is specifically documented not to add duplicates (with
> flags controlling precise de-duping semantics), and in general when all the
> items are "str" it does not, and that seems to be that method's whole
> reason to exist, vs `.append` or whatnot.
>
> The general use case is adding to `$PATH` or similar strings in one's
> `.xonshrc` without ending up with an ever expanding path full of dupes due
> to subshells that launch each other for various reasons.
>
> This all works just fine but the `Path`/`str` mixture seems to break that.
>
> >
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/xonsh/xonsh/issues/4366#issuecomment-881919089>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AELF3BIYPKBQ2DBQEPISGQDTYGSEVANCNFSM5AQJJPRA>
> .
>

Can confirm this is still a thing too. Also even with add used and replace it never removes all copies before inserting the one in front. So you will still have consistent issues.

![image](https://user-images.githubusercontent.com/1596188/132262503-ff91e3c7-aba0-4c73-baa9-964362bec31a.png)
![image](https://user-images.githubusercontent.com/1596188/132262525-70067811-40b9-4a71-9471-69e643824159.png)

Still think the suggestion from: https://github.com/xonsh/xonsh/issues/2562#issuecomment-357891119
Was sleekest way to clean this up after
![image](https://user-images.githubusercontent.com/1596188/132262586-98c3a08b-1bdd-43f4-8aca-017ac4644995.png)

Commit message: Add tests for EnvPath.add() function

Patch to review:
diff --git a/news/fixduplicatepaths.rst b/news/fixduplicatepaths.rst
new file mode 100644
index 0000000000..8beed2482a
--- /dev/null
+++ b/news/fixduplicatepaths.rst
@@ -0,0 +1,15 @@
+**Added:** None
+
+**Changed:**
+
+* <news item>
+
+**Deprecated:** None
+
+**Removed:** None
+
+**Fixed:**
+
+* Fix Duplicate paths left over when add paths to Path via xonsh.tools.EnvPath
+
+**Security:** None
diff --git a/tests/test_tools.py b/tests/test_tools.py
index 6dd1b34049..9784aa9077 100644
--- a/tests/test_tools.py
+++ b/tests/test_tools.py
@@ -928,9 +928,9 @@ def test_env_path_to_str(inp, exp):
     "left, right, exp",
     [
         (
-            EnvPath(["/home/wakka"]),
-            ["/home/jawaka"],
-            EnvPath(["/home/wakka", "/home/jawaka"]),
+                EnvPath(["/home/wakka"]),
+                ["/home/jawaka"],
+                EnvPath(["/home/wakka", "/home/jawaka"]),
         ),
         (["a"], EnvPath(["b"]), EnvPath(["a", "b"])),
         (EnvPath(["c"]), EnvPath(["d"]), EnvPath(["c", "d"])),
@@ -942,6 +942,30 @@ def test_env_path_add(left, right, exp):
     assert exp == obs
 
 
+def test_env_path_add_replace_no_dupes_front_replace_existing():
+    # Test replaces without dupes when added to front when adding existing entry
+    path = EnvPath(["/home/wakka", "/home/wakka/bin"])
+    path.add("/home/wakka/bin", front=True, replace=True)
+    exp = ["/home/wakka/bin", "/home/wakka"]
+    assert exp == path
+
+
+def test_env_path_add_replace_no_dupes_front_replace_multiple():
+    # Test replaces without dupes when added to front when multiple existing occurrences
+    path = EnvPath(["/home/wakka", "/home/wakka/bin", "/home/wakka/bin"])
+    path.add("/home/wakka/bin", front=True, replace=True)
+    exp = ["/home/wakka/bin", "/home/wakka"]
+    assert exp == path
+
+
+def test_env_path_add_replace_no_dupes_back_replace_multiple():
+    # Test replaces without dupes when not added to front
+    path = EnvPath(["/home/wakka", "/home/wakka/bin", "/home/wakka/bin"])
+    path.add("/home/wakka/bin", front=False, replace=True)
+    exp = ["/home/wakka", "/home/wakka/bin"]
+    assert exp == path
+
+
 # helper
 def expand(path):
     return os.path.expanduser(os.path.expandvars(path))
@@ -972,12 +996,12 @@ def test_env_path_getitem(inp, exp, xession, env):
     "inp, exp",
     [
         (
-            os.pathsep.join(["xonsh_dir", "../", ".", "~/"]),
-            ["xonsh_dir", "../", ".", "~/"],
+                os.pathsep.join(["xonsh_dir", "../", ".", "~/"]),
+                ["xonsh_dir", "../", ".", "~/"],
         ),
         (
-            "/home/wakka" + os.pathsep + "/home/jakka" + os.pathsep + "~/",
-            ["/home/wakka", "/home/jakka", "~/"],
+                "/home/wakka" + os.pathsep + "/home/jakka" + os.pathsep + "~/",
+                ["/home/wakka", "/home/jakka", "~/"],
         ),
     ],
 )
@@ -999,8 +1023,8 @@ def test_env_path_multipath(inp, exp, xession, env):
         (pathlib.Path("~/"), ["~"]),
         (pathlib.Path("."), ["."]),
         (
-            ["/home/wakka", pathlib.Path("/home/jakka"), "~/"],
-            ["/home/wakka", "/home/jakka".replace("/", os.sep), "~/"],
+                ["/home/wakka", pathlib.Path("/home/jakka"), "~/"],
+                ["/home/wakka", "/home/jakka".replace("/", os.sep), "~/"],
         ),
         (["/home/wakka", pathlib.Path("../"), "../"], ["/home/wakka", "..", "../"]),
         (["/home/wakka", pathlib.Path("~/"), "~/"], ["/home/wakka", "~", "~/"]),
@@ -1028,8 +1052,8 @@ def mkpath(*paths):
     "inp, exp",
     [
         (
-            [mkpath("home", "wakka"), mkpath("home", "jakka"), mkpath("home", "yakka")],
-            [mkpath("home", "wakka"), mkpath("home", "jakka")],
+                [mkpath("home", "wakka"), mkpath("home", "jakka"), mkpath("home", "yakka")],
+                [mkpath("home", "wakka"), mkpath("home", "jakka")],
         )
     ],
 )
@@ -1042,8 +1066,8 @@ def test_env_path_slice_get_all_except_last_element(inp, exp):
     "inp, exp",
     [
         (
-            [mkpath("home", "wakka"), mkpath("home", "jakka"), mkpath("home", "yakka")],
-            [mkpath("home", "jakka"), mkpath("home", "yakka")],
+                [mkpath("home", "wakka"), mkpath("home", "jakka"), mkpath("home", "yakka")],
+                [mkpath("home", "jakka"), mkpath("home", "yakka")],
         )
     ],
 )
@@ -1056,14 +1080,14 @@ def test_env_path_slice_get_all_except_first_element(inp, exp):
     "inp, exp_a, exp_b",
     [
         (
-            [
-                mkpath("home", "wakka"),
-                mkpath("home", "jakka"),
-                mkpath("home", "yakka"),
-                mkpath("home", "takka"),
-            ],
-            [mkpath("home", "wakka"), mkpath("home", "yakka")],
-            [mkpath("home", "jakka"), mkpath("home", "takka")],
+                [
+                    mkpath("home", "wakka"),
+                    mkpath("home", "jakka"),
+                    mkpath("home", "yakka"),
+                    mkpath("home", "takka"),
+                ],
+                [mkpath("home", "wakka"), mkpath("home", "yakka")],
+                [mkpath("home", "jakka"), mkpath("home", "takka")],
         )
     ],
 )
@@ -1078,14 +1102,14 @@ def test_env_path_slice_path_with_step(inp, exp_a, exp_b):
     "inp, exp",
     [
         (
-            [
-                mkpath("home", "wakka"),
-                mkpath("home", "xakka"),
-                mkpath("other", "zakka"),
-                mkpath("another", "akka"),
-                mkpath("home", "bakka"),
-            ],
-            [mkpath("other", "zakka"), mkpath("another", "akka")],
+                [
+                    mkpath("home", "wakka"),
+                    mkpath("home", "xakka"),
+                    mkpath("other", "zakka"),
+                    mkpath("another", "akka"),
+                    mkpath("home", "bakka"),
+                ],
+                [mkpath("other", "zakka"), mkpath("another", "akka")],
         )
     ],
 )
@@ -1232,8 +1256,8 @@ def test_ensure_slice(inp, exp):
     [
         ((range(50), slice(25, 40)), list(i for i in range(25, 40))),
         (
-            ([1, 2, 3, 4, 5, 6, 7, 8, 9, 10], [slice(1, 4), slice(6, None)]),
-            [2, 3, 4, 7, 8, 9, 10],
+                ([1, 2, 3, 4, 5, 6, 7, 8, 9, 10], [slice(1, 4), slice(6, None)]),
+                [2, 3, 4, 7, 8, 9, 10],
         ),
         (([1, 2, 3, 4, 5], [slice(-2, None), slice(-5, -3)]), [4, 5, 1, 2]),
     ],
@@ -1429,25 +1453,25 @@ def test_escape_windows_cmd_string(st, esc):
         ("", '""', None),
         ("foo", "foo", '"foo"'),
         (
-            r'arg1 "hallo, "world""  "\some\path with\spaces")',
-            r'"arg1 \"hallo, \"world\"\"  \"\some\path with\spaces\")"',
-            None,
+                r'arg1 "hallo, "world""  "\some\path with\spaces")',
+                r'"arg1 \"hallo, \"world\"\"  \"\some\path with\spaces\")"',
+                None,
         ),
         (
-            r'"argument"2" argument3 argument4',
-            r'"\"argument\"2\" argument3 argument4"',
-            None,
+                r'"argument"2" argument3 argument4',
+                r'"\"argument\"2\" argument3 argument4"',
+                None,
         ),
         (r'"\foo\bar bar\foo\" arg', r'"\"\foo\bar bar\foo\\\" arg"', None),
         (
-            r"\\machine\dir\file.bat",
-            r"\\machine\dir\file.bat",
-            r'"\\machine\dir\file.bat"',
+                r"\\machine\dir\file.bat",
+                r"\\machine\dir\file.bat",
+                r'"\\machine\dir\file.bat"',
         ),
         (
-            r'"\\machine\dir space\file.bat"',
-            r'"\"\\machine\dir space\file.bat\""',
-            None,
+                r'"\\machine\dir space\file.bat"',
+                r'"\"\\machine\dir space\file.bat\""',
+                None,
         ),
     ],
 )
@@ -1601,15 +1625,15 @@ def test_expandvars(inp, exp, xession):
         (572392800.0, None, 572392800.0),
         ("42.1459", None, 42.1459),
         (
-            dt.datetime(2016, 8, 2, 13, 24),
-            None,
-            dt.datetime(2016, 8, 2, 13, 24).timestamp(),
+                dt.datetime(2016, 8, 2, 13, 24),
+                None,
+                dt.datetime(2016, 8, 2, 13, 24).timestamp(),
         ),
         ("2016-8-10 16:14", None, dt.datetime(2016, 8, 10, 16, 14).timestamp()),
         (
-            "2016/8/10 16:14:40",
-            "%Y/%m/%d %H:%M:%S",
-            dt.datetime(2016, 8, 10, 16, 14, 40).timestamp(),
+                "2016/8/10 16:14:40",
+                "%Y/%m/%d %H:%M:%S",
+                dt.datetime(2016, 8, 10, 16, 14, 40).timestamp(),
         ),
     ],
 )
@@ -1663,17 +1687,17 @@ def test_swap_values():
     "arguments, expected_docstring",
     [
         (
-            {"deprecated_in": "0.5.10", "removed_in": "0.6.0"},
-            "my_function has been deprecated in version 0.5.10 and will be removed "
-            "in version 0.6.0",
+                {"deprecated_in": "0.5.10", "removed_in": "0.6.0"},
+                "my_function has been deprecated in version 0.5.10 and will be removed "
+                "in version 0.6.0",
         ),
         (
-            {"deprecated_in": "0.5.10"},
-            "my_function has been deprecated in version 0.5.10",
+                {"deprecated_in": "0.5.10"},
+                "my_function has been deprecated in version 0.5.10",
         ),
         (
-            {"removed_in": "0.6.0"},
-            "my_function has been deprecated and will be removed in version 0.6.0",
+                {"removed_in": "0.6.0"},
+                "my_function has been deprecated and will be removed in version 0.6.0",
         ),
         ({}, "my_function has been deprecated"),
     ],
@@ -1690,18 +1714,18 @@ def my_function():
     "arguments, expected_docstring",
     [
         (
-            {"deprecated_in": "0.5.10", "removed_in": "0.6.0"},
-            "Does nothing.\n\nmy_function has been deprecated in version 0.5.10 and "
-            "will be removed in version 0.6.0",
+                {"deprecated_in": "0.5.10", "removed_in": "0.6.0"},
+                "Does nothing.\n\nmy_function has been deprecated in version 0.5.10 and "
+                "will be removed in version 0.6.0",
         ),
         (
-            {"deprecated_in": "0.5.10"},
-            "Does nothing.\n\nmy_function has been deprecated in version 0.5.10",
+                {"deprecated_in": "0.5.10"},
+                "Does nothing.\n\nmy_function has been deprecated in version 0.5.10",
         ),
         (
-            {"removed_in": "0.6.0"},
-            "Does nothing.\n\nmy_function has been deprecated and will be removed "
-            "in version 0.6.0",
+                {"removed_in": "0.6.0"},
+                "Does nothing.\n\nmy_function has been deprecated and will be removed "
+                "in version 0.6.0",
         ),
         ({}, "Does nothing.\n\nmy_function has been deprecated"),
     ],
@@ -1827,19 +1851,19 @@ def test_all_permutations():
     [
         ("test1", {}, {}),  # empty styles
         (
-            "test2",
-            {"Token.Literal.String.Single": "#ff0000"},
-            {"Token.Literal.String.Single": "#ff0000"},
+                "test2",
+                {"Token.Literal.String.Single": "#ff0000"},
+                {"Token.Literal.String.Single": "#ff0000"},
         ),  # str key
         (
-            "test3",
-            {"Literal.String.Single": "#ff0000"},
-            {"Token.Literal.String.Single": "#ff0000"},
+                "test3",
+                {"Literal.String.Single": "#ff0000"},
+                {"Token.Literal.String.Single": "#ff0000"},
         ),  # short str key
         (
-            "test4",
-            {"RED": "#ff0000"},
-            {"Token.Color.RED": "#ff0000"},
+                "test4",
+                {"RED": "#ff0000"},
+                {"Token.Color.RED": "#ff0000"},
         ),  # color
     ],
 )
diff --git a/xonsh/tools.py b/xonsh/tools.py
index 359c9295aa..36e2a04f89 100644
--- a/xonsh/tools.py
+++ b/xonsh/tools.py
@@ -283,7 +283,8 @@ def add(self, data, front=False, replace=False):
         if data not in self._l:
             self._l.insert(0 if front else len(self._l), data)
         elif replace:
-            self._l.remove(data)
+            # 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)
 
 


Code review comments:
diff_hunk: @@ -942,6 +942,30 @@ def test_env_path_add(left, right, exp):
     assert exp == obs
 
 
+def test_env_path_add_replace_no_dupes_front_replace_existing():
+    # Test replaces without dupes when added to front when adding existing entry
+    path = EnvPath(["/home/wakka", "/home/wakka/bin"])
+    path.add("/home/wakka/bin", front=True, replace=True)
+    exp = ["/home/wakka/bin", "/home/wakka"]
+    assert exp == path
path: tests/test_tools.py
text: @user1:
Inlining `exp` will make the tests clearer:
```python
assert path == ["/home/wakka/bin", "/home/wakka"]
```