Repository: drivendataorg/cloudpathlib (python)

Issue:
Don't monkeypatch Path.resolve (and simplify resolve)
Currently we have this [heavily-modified copy of `resolve` from pathlib](https://github.com/drivendataorg/cloudpathlib/blob/604fdec5b82a3c2665d977827d49265529a7ae55/cloudpathlib/_vendored.py#L16) which gets used when [`CloudPath` dispatches to `PurePosixPath` methods](https://github.com/drivendataorg/cloudpathlib/blob/604fdec5b82a3c2665d977827d49265529a7ae55/cloudpathlib/cloudpath.py#L398-L405). 

This has some potential to be simplified. We should revisit this issue when there is a more robust test suite. 

### (A) Don't monkeypatch, just call as a function

Current this method is monkeypatched onto `PurePosixPath` but this has the side-effect that it actually modifies `PurePosixPath` for users. This isn't really necessarily, because no internal methods of `PurePosixPath` will call it, since it doesn't normally exist. We can just call it as a function in `CloudPath` methods as needed.

### (B) Simplify logic

Resolve does a few things:

1. Make path absolute by ensuring a single leading `/`
2. Collapse multiple slashes into one, e.g., `a//b` -> `a/b`
3. Handle `.` and `..`

(2) already happens for free on `PurePosixPath` instantiation so it's not really necessary:

```
>>> PurePosixPath("foo//////bar")
PurePosixPath('foo/bar')
```

It's not clear that (3) is a desirable mechanism for `CloudPaths` and it may be a bug that we do something with them.

If all we need is to handle (1), then this can be done more simply. 

Pull request:
Simplify resolve
- Simplifies `resolve`: remove commented stuff and unneeded function layer
- Don't monkeypatch resolve onto `PurePosixPath`: we don't want to change that class for users
- Get mypy passing and add it to lint

Resolves #45, #43  

Patch:
diff --git a/Makefile b/Makefile
index 6232ef9..1ffd56d 100644
--- a/Makefile
+++ b/Makefile
@@ -54,6 +54,7 @@ install: clean ## install the package to the active Python's site-packages
 lint: ## check style with flake8
 	black --check cloudpathlib tests docs
 	flake8 cloudpathlib tests docs
+	mypy cloudpathlib
 
 release: dist ## package and upload a release
 	twine upload dist/*
diff --git a/cloudpathlib/_vendored.py b/cloudpathlib/_vendored.py
deleted file mode 100644
index 7b276b2..0000000
--- a/cloudpathlib/_vendored.py
+++ /dev/null
@@ -1,61 +0,0 @@
-## Vendored functions
-##
-## There are a few functions that we vendor the bulk of the logic from the
-## pathlib source since they are not available on the objects that we use; :'(
-##   
-
-# The function resolve is not available on Pure paths because it removes relative
-# paths and symlinks. We _just_ want the relative path resolution for
-# cloud paths, so the other logic is removed.  Also, we can assume that
-# cloud paths are absolute.
-#
-# Left as much as possible unchanged
-#
-# Adapted from:
-# https://github.com/python/cpython/blob/3.8/Lib/pathlib.py#L316-L359
-def resolve(path, strict=False):
-    # sep = self.sep
-    sep = "/"
-    # accessor = path._accessor
-    # seen = {}
-
-    def _resolve(path, rest):
-        if rest.startswith(sep):
-            path = ''
-
-        for name in rest.split(sep):
-            if not name or name == '.':
-                # current dir
-                continue
-            if name == '..':
-                # parent dir
-                path, _, _ = path.rpartition(sep)
-                continue
-            newpath = path + sep + name
-            # if newpath in seen:
-            #     # Already seen this path
-            #     path = seen[newpath]
-            #     if path is not None:
-            #         # use cached value
-            #         continue
-            #     # The symlink is not resolved, so we must have a symlink loop.
-            #     raise RuntimeError("Symlink loop from %r" % newpath)
-            # # Resolve the symbolic link
-            # try:
-            #     target = accessor.readlink(newpath)
-            # except OSError as e:
-            #     if e.errno != EINVAL and strict:
-            #         raise
-            #     # Not a symlink, or non-strict mode. We just leave the path
-            #     # untouched.
-            path = newpath
-            # else:
-            #     seen[newpath] = None # not resolved symlink
-            #      path = _resolve(path, newpath)
-            #     seen[newpath] = path # resolved symlink
-
-        return path
-
-    # NOTE: according to POSIX, getcwd() cannot contain path components
-    # which are symlinks.
-    return _resolve('', str(path)) or sep
diff --git a/cloudpathlib/azure/azblobclient.py b/cloudpathlib/azure/azblobclient.py
index 9c73018..32c6a3a 100644
--- a/cloudpathlib/azure/azblobclient.py
+++ b/cloudpathlib/azure/azblobclient.py
@@ -208,4 +208,4 @@ class AzureBlobClient(Client):
         return cloud_path
 
 
-AzureBlobClient.AzureBlobPath = AzureBlobClient.CloudPath
+AzureBlobClient.AzureBlobPath = AzureBlobClient.CloudPath  # type: ignore
diff --git a/cloudpathlib/cloudpath.py b/cloudpathlib/cloudpath.py
index 474fcb5..09ad1ac 100644
--- a/cloudpathlib/cloudpath.py
+++ b/cloudpathlib/cloudpath.py
@@ -8,10 +8,6 @@ from typing import Any, IO, Iterable, Union
 from urllib.parse import urlparse
 from warnings import warn
 
-from ._vendored import resolve
-
-PurePosixPath.resolve = resolve
-
 
 # Custom Exceptions
 class MissingDependencies(Exception):
@@ -454,14 +450,14 @@ class CloudPath(metaclass=CloudPathMeta):
         # Paths should always be resolved and then converted to the same client + class as this one
         if isinstance(path_version, PurePosixPath):
             # always resolve since cloud paths must be absolute
-            path_version = path_version.resolve()
+            path_version = _resolve(path_version)
             return self._new_cloudpath(path_version)
 
         if isinstance(path_version, collections.abc.Iterable) and isinstance(
             path_version[0], PurePosixPath
         ):
             return [
-                self._new_cloudpath(p.resolve()) for p in path_version if p.resolve() != p.root
+                self._new_cloudpath(_resolve(p)) for p in path_version if _resolve(p) != p.root
             ]
 
         # when pathlib returns a string, etc. we probably just want that thing
@@ -679,3 +675,28 @@ class CloudPath(metaclass=CloudPathMeta):
             f"(2) pass `force_overwrite_to_cloud=True` to "
             f"overwrite."
         )
+
+
+# The function resolve is not available on Pure paths because it removes relative
+# paths and symlinks. We _just_ want the relative path resolution for
+# cloud paths, so the other logic is removed.  Also, we can assume that
+# cloud paths are absolute.
+#
+# Based on resolve from pathlib:
+# https://github.com/python/cpython/blob/3.8/Lib/pathlib.py#L316-L359
+def _resolve(path: PurePosixPath) -> str:
+    sep = "/"
+
+    # rebuild path from parts
+    newpath = ""
+    for name in str(path).split(sep):
+        if not name or name == ".":
+            # current dir, nothing to add
+            continue
+        if name == "..":
+            # parent dir, drop right-most part
+            newpath, _, _ = newpath.rpartition(sep)
+            continue
+        newpath = newpath + sep + name
+
+    return newpath or sep
diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py
index 1254e98..8b7c311 100644
--- a/cloudpathlib/s3/s3client.py
+++ b/cloudpathlib/s3/s3client.py
@@ -189,4 +189,4 @@ class S3Client(Client):
         return cloud_path
 
 
-S3Client.S3Path = S3Client.CloudPath
+S3Client.S3Path = S3Client.CloudPath  # type: ignore


Test patch:
diff --git a/tests/test_cloudpath_internal.py b/tests/test_cloudpath_internal.py
new file mode 100644
index 0000000..5158119
--- /dev/null
+++ b/tests/test_cloudpath_internal.py
@@ -0,0 +1,18 @@
+from pathlib import PurePosixPath
+
+from cloudpathlib.cloudpath import _resolve
+
+
+def test_resolve():
+    assert _resolve(PurePosixPath("/a/b/c")) == "/a/b/c"
+    assert _resolve(PurePosixPath("/a/b/c/")) == "/a/b/c"
+    assert _resolve(PurePosixPath("/a/b/c//")) == "/a/b/c"
+    assert _resolve(PurePosixPath("//a/b/c//")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a/b/c")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a/b/c/")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a//b/c")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a////b/c")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a/b/./c")) == "/a/b/c"
+    assert _resolve(PurePosixPath("a/b/../c")) == "/a/c"
+    assert _resolve(PurePosixPath("a/b/../../c")) == "/c"
+    assert _resolve(PurePosixPath("")) == "/"