👕 address review comments

fs/_url_tools.py

@@ -0,0 +1,47 @@
+import re
+import six
+import platform
+
+
+_WINDOWS_PLATFORM = platform.system() == "Windows"
+
+
+def url_quote(path_snippet):
+    """
+    On Windows, it will separate drive letter and quote windows
+    path alone. No magic on Unix-alie path, just pythonic
+    `pathname2url`
+
+    Arguments:
+       path_snippet: a file path, relative or absolute.
+    """
+    # type: (Text) -> Text
+    if _WINDOWS_PLATFORM and _has_drive_letter(path_snippet):
+        drive_letter, path = path_snippet.split(":", 1)
+        if six.PY2:
+            path = path.encode("utf-8")
+        path = six.moves.urllib.request.pathname2url(path)
+        path_snippet = "{}:{}".format(drive_letter, path)
+    else:
+        if six.PY2:
+            path_snippet = path_snippet.encode("utf-8")
+        path_snippet = six.moves.urllib.request.pathname2url(path_snippet)
+    return path_snippet
+
+
+def _has_drive_letter(path_snippet):
+    """
+    The following path will get True
+    D:/Data
+    C:\\My Dcouments\\ test
+
+    And will get False
+
+    /tmp/abc:test
+
+    Arguments:
+       path_snippet: a file path, relative or absolute.
+    """
+    # type: (Text) -> Text
+    windows_drive_pattern = ".:[/\\\\].*$"
+    return re.match(windows_drive_pattern, path_snippet) is not None

fs/base.py

@@ -8,15 +8,13 @@
 
 from __future__ import absolute_import, print_function, unicode_literals
 
-import re
 import abc
 import hashlib
 import itertools
 import os
 import threading
 import time
 import typing
-import platform
 from contextlib import closing
 from functools import partial, wraps
 import warnings
@@ -63,7 +61,6 @@
 
 
 __all__ = ["FS"]
-_WINDOWS_PLATFORM = platform.system() == "Windows"
 
 
 def _new_name(method, old_name):
@@ -313,20 +310,6 @@ def setinfo(self, path, info):
 
         """
 
-    @staticmethod
-    def quote(path_snippet):
-        if _WINDOWS_PLATFORM and has_drive_letter(path_snippet):
-            drive_letter, path = path_snippet.split(":", 1)
-            if six.PY2:
-                path = path.encode("utf-8")
-            path = six.moves.urllib.request.pathname2url(path)
-            path_snippet = "{}:{}".format(drive_letter, path)
-        else:
-            if six.PY2:
-                path_snippet = path_snippet.encode("utf-8")
-            path_snippet = six.moves.urllib.request.pathname2url(path_snippet)
-        return path_snippet
-
     # ---------------------------------------------------------------- #
     # Optional methods                                                 #
     # Filesystems *may* implement these methods.                       #
@@ -1662,8 +1645,3 @@ def hash(self, path, name):
                     break
                 hash_object.update(chunk)
         return hash_object.hexdigest()
-
-
-def has_drive_letter(path_snippet):
-    windows_drive_pattern = ".:[/\\\\].*$"
-    return re.match(windows_drive_pattern, path_snippet) is not None

fs/osfs.py

@@ -48,6 +48,7 @@
 from .error_tools import convert_os_errors
 from .mode import Mode, validate_open_mode
 from .errors import FileExpected, NoURL
+from ._url_tools import url_quote
 
 if False:  # typing.TYPE_CHECKING
     from typing import (
@@ -136,7 +137,8 @@ def __init__(
                 )
         else:
             if not os.path.isdir(_root_path):
-                raise errors.CreateFailed("'{}' does not exist".format(_root_path))
+                message = "root path '{}' does not exist".format(_root_path)
+                raise errors.CreateFailed(message)
 
         _meta = self._meta = {
             "network": False,
@@ -586,7 +588,7 @@ def geturl(self, path, purpose="download"):
         if purpose == "download":
             return "file://" + self.getsyspath(path)
         elif purpose == "fs":
-            url_path = FS.quote(sys_path)
+            url_path = url_quote(sys_path)
             return "osfs://" + url_path
         else:
             raise NoURL(path, purpose)

fs/tarfs.py

@@ -23,7 +23,7 @@
 from .path import relpath, basename, isbase, normpath, parts, frombase
 from .wrapfs import WrapFS
 from .permissions import Permissions
-
+from ._url_tools import url_quote
 
 if False:  # typing.TYPE_CHECKING
     from tarfile import TarInfo
@@ -470,8 +470,8 @@ def isclosed(self):
     def geturl(self, path, purpose="download"):
         # type: (Text, Text) -> Text
         if purpose == "fs":
-            quoted_file = FS.quote(self._file)
-            quoted_path = FS.quote(path)
+            quoted_file = url_quote(self._file)
+            quoted_path = url_quote(path)
             return "tar://{}!/{}".format(quoted_file, quoted_path)
         else:
             raise NoURL(path, purpose)

fs/zipfs.py

@@ -22,6 +22,7 @@
 from .path import dirname, forcedir, normpath, relpath
 from .time import datetime_to_epoch
 from .wrapfs import WrapFS
+from ._url_tools import url_quote
 
 if False:  # typing.TYPE_CHECKING
     from typing import (
@@ -449,8 +450,8 @@ def readbytes(self, path):
     def geturl(self, path, purpose="download"):
         # type: (Text, Text) -> Text
         if purpose == "fs":
-            quoted_file = FS.quote(self._file)
-            quoted_path = FS.quote(path)
+            quoted_file = url_quote(self._file)
+            quoted_path = url_quote(path)
             return "zip://{}!/{}".format(quoted_file, quoted_path)
         else:
             raise errors.NoURL(path, purpose)

tests/test_base.py

@@ -4,7 +4,6 @@
 from __future__ import unicode_literals
 
 import unittest
-import platform
 
 try:
     import mock
@@ -71,32 +70,3 @@ def mock_getsyspath(path):
 
         with self.assertRaises(errors.InvalidPath):
             self.fs.validatepath("0123456789A")
-
-    def test_quote(self):
-        test_fixtures = [
-            # test_snippet, expected
-            ["foo/bar/egg/foofoo", "foo/bar/egg/foofoo"],
-            ["foo/bar ha/barz", "foo/bar%20ha/barz"],
-            ["example b.txt", "example%20b.txt"],
-            ["exampleㄓ.txt", "example%E3%84%93.txt"],
-        ]
-        if platform.system() == "Windows":
-            test_fixtures.extend(
-                [
-                    ["C:\\My Documents\\test.txt", "C:/My%20Documents/test.txt"],
-                    ["C:/My Documents/test.txt", "C:/My%20Documents/test.txt"],
-                    # on Windows '\' is regarded as path separator
-                    ["test/forward\\slash", "test/forward/slash"],
-                ]
-            )
-        else:
-            test_fixtures.extend(
-                [
-                    # colon:tmp is bad path under Windows
-                    ["test/colon:tmp", "test/colon%3Atmp"],
-                    # Unix treat \ as %5C
-                    ["test/forward\\slash", "test/forward%5Cslash"],
-                ]
-            )
-        for test_snippet, expected in test_fixtures:
-            self.assertEqual(FS.quote(test_snippet), expected)

tests/test_url_tools.py

@@ -0,0 +1,35 @@
+import platform
+import unittest
+
+from fs._url_tools import url_quote
+
+
+class TestBase(unittest.TestCase):
+    def test_quote(self):
+        test_fixtures = [
+            # test_snippet, expected
+            ["foo/bar/egg/foofoo", "foo/bar/egg/foofoo"],
+            ["foo/bar ha/barz", "foo/bar%20ha/barz"],
+            ["example b.txt", "example%20b.txt"],
+            ["exampleㄓ.txt", "example%E3%84%93.txt"],
+        ]
+        if platform.system() == "Windows":
+            test_fixtures.extend(
+                [
+                    ["C:\\My Documents\\test.txt", "C:/My%20Documents/test.txt"],
+                    ["C:/My Documents/test.txt", "C:/My%20Documents/test.txt"],
+                    # on Windows '\' is regarded as path separator
+                    ["test/forward\\slash", "test/forward/slash"],
+                ]
+            )
+        else:
+            test_fixtures.extend(
+                [
+                    # colon:tmp is bad path under Windows
+                    ["test/colon:tmp", "test/colon%3Atmp"],
+                    # Unix treat \ as %5C
+                    ["test/forward\\slash", "test/forward%5Cslash"],
+                ]
+            )
+        for test_snippet, expected in test_fixtures:
+            self.assertEqual(url_quote(test_snippet), expected)