better handling of ftp features (#91)

* better handling of ftp features

* linter cleanup

* changelog

* e -> error

* call base class

Author: willmcgugan

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - settext, appendtext, appendbytes, setbytes now raise a TypeError if
   the type is wrong, rather than ValueError
+- More efficient feature detection for FTPFS
+- Fixes for `fs.filesize`
 
 ## [2.0.11]
 

fs/ftpfs.py

@@ -10,7 +10,6 @@
 
 from contextlib import contextmanager
 from ftplib import FTP
-from ftplib import error_reply
 from ftplib import error_perm
 from ftplib import error_temp
 
@@ -44,16 +43,18 @@ def ftp_errors(fs, path=None):
         raise errors.RemoteConnectionError(
             msg='unable to connect to {}'.format(fs.host)
         )
-    except error_temp as e:
+    except error_temp as error:
         if path is not None:
             raise errors.ResourceError(
                 path,
-                msg="ftp error on resource '{}' ({})".format(path, e)
+                msg="ftp error on resource '{}' ({})".format(path, error)
             )
         else:
-            raise errors.OperationFailed(msg='ftp error ({})'.format(e))
-    except error_perm as e:
-        code, message = parse_ftp_error(e)
+            raise errors.OperationFailed(
+                msg='ftp error ({})'.format(error)
+            )
+    except error_perm as error:
+        code, message = _parse_ftp_error(error)
         if code == 552:
             raise errors.InsufficientStorage(
                 path=path,
@@ -76,8 +77,10 @@ def manage_ftp(ftp):
         except:  # pragma: nocover
             pass
 
-def parse_ftp_error(e):
-    code, _, message = text_type(e).partition(' ')
+
+def _parse_ftp_error(error):
+    """Extract code and message from ftp error."""
+    code, _, message = text_type(error).partition(' ')
     if code.isdigit():
         code = int(code)
     return code, message
@@ -98,6 +101,7 @@ def _decode(st, _):
 class FTPFile(io.IOBase):
 
     def __init__(self, ftpfs, path, mode):
+        super(FTPFile, self).__init__()
         self.fs = ftpfs
         self.path = path
         self.mode = Mode(mode)
@@ -108,7 +112,8 @@ def __init__(self, ftpfs, path, mode):
         self._write_conn = None
 
     def _open_ftp(self):
-        ftp = self.fs._open_ftp(self.fs.ftp.encoding)
+        """Open an ftp object for the file."""
+        ftp = self.fs._open_ftp()
         ftp.voidcmd(str('TYPE I'))
         return ftp
 
@@ -302,6 +307,7 @@ def __init__(self,
         self.timeout = timeout
         self.port = port
 
+        self.encoding = 'latin-1'
         self._ftp = None
         self._welcome = None
         self._features = None
@@ -317,21 +323,52 @@ def __str__(self):
         )
         return _fmt.format(host=self.host, port=self.port)
 
-    def _open_ftp(self, encoding="utf-8"):
+    @classmethod
+    def _parse_features(cls, feat_response):
+        """Parse a dict of features from FTP feat response."""
+        features = {}
+        if feat_response.split('-')[0] == '211':
+            for line in feat_response.splitlines():
+                if line.startswith(' '):
+                    key, _, value = line[1:].partition(' ')
+                    features[key] = value
+        return features
+
+    def _open_ftp(self):
+        """Open a new ftp object."""
         _ftp = FTP()
         _ftp.set_debuglevel(0)
         with ftp_errors(self):
-            _ftp.encoding = encoding
             _ftp.connect(self.host, self.port, self.timeout)
             _ftp.login(self.user, self.passwd, self.acct)
+            self._features = {}
+            try:
+                feat_response = _decode(_ftp.sendcmd("FEAT"), 'latin-1')
+            except error_perm:
+                self.encoding = 'latin-1'
+            else:
+                self._features = self._parse_features(feat_response)
+                self.encoding = (
+                    'utf-8'
+                    if 'UTF8' in self._features
+                    else 'latin-1'
+                )
+                if not PY2:
+                    _ftp.file = _ftp.sock.makefile(
+                        'r',
+                        encoding=self.encoding
+                    )
+        _ftp.encoding = self.encoding
+        self._welcome = _ftp.welcome
         return _ftp
 
     def _manage_ftp(self):
-        ftp = self._open_ftp(self.ftp.encoding)
+        ftp = self._open_ftp()
         return manage_ftp(ftp)
 
     @property
     def ftp_url(self):
+        """Get the FTP url this filesystem will open."""
         url = (
             "ftp://{}".format(self.host)
             if self.port == 21
@@ -342,39 +379,17 @@ def ftp_url(self):
     @property
     def ftp(self):
         """Get a FTP (ftplib) object."""
+        return self._get_ftp()
+
+    def _get_ftp(self):
         if self._ftp is None:
-            _ftp = self._open_ftp('latin-1')
-            try:
-                encoding = (
-                    'utf-8'
-                    if 'UTF8' in _ftp.sendcmd('FEAT')
-                    else 'latin-1'
-                )
-            except error_perm:
-                encoding = 'latin-1'
-            self._ftp = (
-                _ftp
-                if encoding == 'latin-1'
-                else self._open_ftp(encoding)
-            )
-            self._welcome = self._ftp.getwelcome()
+            self._ftp = self._open_ftp()
         return self._ftp
 
     @property
     def features(self):
         """Get features dict from FTP server."""
-        if self._features is None:
-            try:
-                response = _decode(self.ftp.sendcmd("FEAT"), "ascii")
-            except error_perm:
-                self._features = {}
-            else:
-                self._features = {}
-                if response.split('-')[0] == '211':
-                    for line in response.splitlines():
-                        if line.startswith(' '):
-                            k, _, v = line[1:].partition(' ')
-                            self._features[k] = v
+        self._get_ftp()
         return self._features
 
     def _read_dir(self, path):
@@ -395,6 +410,7 @@ def _read_dir(self, path):
 
     @property
     def supports_mlst(self):
+        """Check if server supports MLST feature."""
         return 'MLST' in self.features
 
     def create(self, path, wipe=False):
@@ -408,14 +424,15 @@ def create(self, path, wipe=False):
                 )
 
     @classmethod
-    def _parse_ftp_time(cls, t):
+    def _parse_ftp_time(cls, time_text):
+        """Parse a time from an ftp directory listing."""
         try:
-            tm_year = int(t[0:4])
-            tm_month = int(t[4:6])
-            tm_day = int(t[6:8])
-            tm_hour = int(t[8:10])
-            tm_min = int(t[10:12])
-            tm_sec = int(t[12:14])
+            tm_year = int(time_text[0:4])
+            tm_month = int(time_text[4:6])
+            tm_day = int(time_text[6:8])
+            tm_hour = int(time_text[8:10])
+            tm_min = int(time_text[10:12])
+            tm_sec = int(time_text[12:14])
         except ValueError:
             return None
         epoch_time = calendar.timegm((
@@ -433,11 +450,11 @@ def _parse_facts(cls, line):
         name = None
         facts = {}
         for fact in line.split(';'):
-            k, sep, v = fact.partition('=')
+            key, sep, value = fact.partition('=')
             if sep:
-                k = k.strip().lower()
-                v = v.strip()
-                facts[k] = v
+                key = key.strip().lower()
+                value = value.strip()
+                facts[key] = value
             else:
                 name = basename(fact.rstrip('/').strip())
         return name if name not in ('.', '..') else None, facts
@@ -514,6 +531,7 @@ def getinfo(self, path, namespaces=None):
 
     def getmeta(self, namespace="standard"):
         _meta = {}
+        self._get_ftp()
         if namespace == "standard":
             _meta = self._meta.copy()
             _meta['unicode_paths'] = "UTF8" in self.features
@@ -542,8 +560,8 @@ def makedir(self, path, permissions=None, recreate=False):
             if not (recreate and self.isdir(path)):
                 try:
                     self.ftp.mkd(_encode(_path, self.ftp.encoding))
-                except error_perm as e:
-                    code, _ = parse_ftp_error(e)
+                except error_perm as error:
+                    code, _ = _parse_ftp_error(error)
                     if code == 550:
                         if self.isdir(path):
                             raise errors.DirectoryExists(path)
@@ -569,13 +587,12 @@ def openbin(self, path, mode="r", buffering=-1, **options):
                     raise errors.FileExpected(path)
             if _mode.exclusive:
                 raise errors.FileExists(path)
-            f = FTPFile(self, _path, mode)
-        return f
+            ftp_file = FTPFile(self, _path, mode)
+        return ftp_file
 
     def remove(self, path):
         self.check()
         _path = self.validatepath(path)
-        dir_name, file_name = split(_path)
         with self._lock:
             if self.isdir(path):
                 raise errors.FileExpected(path=path)
@@ -586,13 +603,12 @@ def removedir(self, path):
         _path = self.validatepath(path)
         if _path == '/':
             raise errors.RemoveRootError()
-        _dir_name, file_name = split(_path)
 
         with ftp_errors(self, path):
             try:
                 self.ftp.rmd(_encode(_path, self.ftp.encoding))
-            except error_perm as e:
-                code, _ = parse_ftp_error(e)
+            except error_perm as error:
+                code, _ = _parse_ftp_error(error)
                 if code == 550:
                     if self.isfile(path):
                         raise errors.DirectoryExpected(path)
@@ -611,7 +627,7 @@ def _scandir(self, path, namespaces=None):
                             str("MLSD ") + _encode(_path, self.ftp.encoding),
                             lambda l: lines.append(_decode(l, self.ftp.encoding))
                         )
-                    except error_perm as e:
+                    except error_perm:
                         if not self.getinfo(path).is_dir:
                             raise errors.DirectoryExpected(path)
                         raise # pragma: no cover
@@ -661,8 +677,8 @@ def getbytes(self, path):
                         str("RETR ") + _encode(_path, self.ftp.encoding),
                         data.write
                     )
-                except error_perm as e:
-                    code, _ = parse_ftp_error(e)
+                except error_perm as error:
+                    code, _ = _parse_ftp_error(error)
                     if code == 550:
                         if self.isdir(path):
                             raise errors.FileExpected(path)
@@ -679,8 +695,3 @@ def close(self):
                 pass
             self._ftp = None
         super(FTPFS, self).close()
-
-
-if __name__ == "__main__":  # pragma: no cover
-    fs = FTPFS('ftp.mirror.nl', 'anonymous', '[email protected]')
-    print(list(fs.scandir('/')))

tests/test_ftpfs.py

@@ -143,12 +143,6 @@ def test_connection_error(self):
         with self.assertRaises(errors.RemoteConnectionError):
             fs.open('foo.txt')
 
-    def test_features(self):
-        def broken_sendcmd(cmd):
-            raise ftplib.error_perm('nope')
-        self.fs.ftp.sendcmd = broken_sendcmd
-        self.assertEqual(self.fs.features, {})
-
     def test_getmeta_unicode_path(self):
         self.assertTrue(self.fs.getmeta().get('unicode_paths'))
         self.fs.features