From b42d5ef0df26ef3f55f4832c953e965d85844e26 Mon Sep 17 00:00:00 2001 From: atollk Date: Thu, 15 Apr 2021 09:28:19 +0200 Subject: [PATCH 1/8] Added support for "last modified time" in `getinfo`. A new namespace "modified" was introduced to the `getinfo` function to potentially use cheaper commands compared to the general "details" namespace. In particular, `FTPFS` now uses the MDTM command if supported by the server. --- CHANGELOG.md | 6 ++++++ fs/base.py | 21 +++++++++++++++++++++ fs/copy.py | 10 ++++------ fs/ftpfs.py | 14 ++++++++++++++ fs/info.py | 9 ++++++--- fs/osfs.py | 1 + tests/test_ftpfs.py | 19 +++++++++++++++++-- tests/test_memoryfs.py | 1 - tests/test_opener.py | 16 ++++++++++++++-- tests/test_wrap.py | 8 ++++---- 10 files changed, 87 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f933fb1..e210d618 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `fs.copy.copy_file_if`, `fs.copy.copy_dir_if`, and `fs.copy.copy_fs_if`. Closes [#458](https://github.com/PyFilesystem/pyfilesystem2/issues/458). +### Changed + +- FTP servers that do not support the MLST command now try to use the MDTM command to + retrieve the last modification timestamp of a resource. + Closes [#456](https://github.com/PyFilesystem/pyfilesystem2/pull/456). + ### Fixed - Fixed performance bugs in `fs.copy.copy_dir_if_newer`. Test cases were adapted to catch those bugs in the future. diff --git a/fs/base.py b/fs/base.py index 4966d0ca..0e4d8fed 100644 --- a/fs/base.py +++ b/fs/base.py @@ -678,6 +678,25 @@ def readtext( gettext = _new_name(readtext, "gettext") + def getmodified(self, path): + # type: (Text) -> Optional[datetime] + """Get the timestamp of the last modifying access of a resource. + + Arguments: + path (str): A path to a resource. + + Returns: + datetime: The timestamp of the last modification. + + The *modified timestamp* of a file is the point in time + that the file was last changed. Depending on the file system, + it might only have limited accuracy. + + """ + if self.getmeta().get("supports_mtime", False): + return self.getinfo(path, namespaces=["modified"]).modified + return self.getinfo(path, namespaces=["details"]).modified + def getmeta(self, namespace="standard"): # type: (Text) -> Mapping[Text, object] """Get meta information regarding a filesystem. @@ -715,6 +734,8 @@ def getmeta(self, namespace="standard"): read_only `True` if this filesystem is read only. supports_rename `True` if this filesystem supports an `os.rename` operation. + supports_mtime `True` if this filesystem supports a native + operation to retreive the "last modified" time. =================== ============================================ Most builtin filesystems will provide all these keys, and third- diff --git a/fs/copy.py b/fs/copy.py index 03108c00..9a6b9b62 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -398,9 +398,8 @@ def _copy_is_necessary( elif condition == "newer": try: - namespace = ("details",) - src_modified = src_fs.getinfo(src_path, namespace).modified - dst_modified = dst_fs.getinfo(dst_path, namespace).modified + src_modified = src_fs.getmodified(src_path) + dst_modified = dst_fs.getmodified(dst_path) except ResourceNotFound: return True else: @@ -412,9 +411,8 @@ def _copy_is_necessary( elif condition == "older": try: - namespace = ("details",) - src_modified = src_fs.getinfo(src_path, namespace).modified - dst_modified = dst_fs.getinfo(dst_path, namespace).modified + src_modified = src_fs.getmodified(src_path) + dst_modified = dst_fs.getmodified(dst_path) except ResourceNotFound: return True else: diff --git a/fs/ftpfs.py b/fs/ftpfs.py index 515709df..459749c4 100644 --- a/fs/ftpfs.py +++ b/fs/ftpfs.py @@ -15,6 +15,7 @@ from contextlib import contextmanager from ftplib import FTP + try: from ftplib import FTP_TLS except ImportError as err: @@ -667,6 +668,18 @@ def getinfo(self, path, namespaces=None): } ) + if "modified" in namespaces: + if "basic" in namespaces or "details" in namespaces: + raise ValueError( + 'Cannot use the "modified" namespace in combination with others.' + ) + with self._lock: + with ftp_errors(self, path=path): + cmd = "MDTM " + _encode(self.validatepath(path), self.ftp.encoding) + response = self.ftp.sendcmd(cmd) + modified_info = {"modified": self._parse_ftp_time(response.split()[1])} + return Info({"modified": modified_info}) + if self.supports_mlst: with self._lock: with ftp_errors(self, path=path): @@ -692,6 +705,7 @@ def getmeta(self, namespace="standard"): if namespace == "standard": _meta = self._meta.copy() _meta["unicode_paths"] = "UTF8" in self.features + _meta["supports_mtime"] = "MDTM" in self.features return _meta def listdir(self, path): diff --git a/fs/info.py b/fs/info.py index 03bf27cd..6c6cdb0e 100644 --- a/fs/info.py +++ b/fs/info.py @@ -317,9 +317,12 @@ def modified(self): namespace is not in the Info. """ - self._require_namespace("details") - _time = self._make_datetime(self.get("details", "modified")) - return _time + try: + self._require_namespace("details") + return self._make_datetime(self.get("details", "modified")) + except MissingInfoNamespace: + self._require_namespace("modified") + return self._make_datetime(self.get("modified", "modified")) @property def created(self): diff --git a/fs/osfs.py b/fs/osfs.py index 5beb16bf..d5883948 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -144,6 +144,7 @@ def __init__( "network": False, "read_only": False, "supports_rename": True, + "supports_mtime": False, "thread_safe": True, "unicode_paths": os.path.supports_unicode_filenames, "virtual": False, diff --git a/tests/test_ftpfs.py b/tests/test_ftpfs.py index b0e0c1f0..37bb340b 100644 --- a/tests/test_ftpfs.py +++ b/tests/test_ftpfs.py @@ -144,7 +144,6 @@ def test_manager_with_host(self): @mark.slow @unittest.skipIf(platform.python_implementation() == "PyPy", "ftp unreliable with PyPy") class TestFTPFS(FSTestCases, unittest.TestCase): - user = "user" pasw = "1234" @@ -243,6 +242,23 @@ def test_getmeta_unicode_path(self): del self.fs.features["UTF8"] self.assertFalse(self.fs.getmeta().get("unicode_paths")) + def test_getinfo_modified(self): + self.assertIn("MDTM", self.fs.features) + self.fs.create("bar") + mtime_detail = self.fs.getinfo("bar", ("basic", "details")).modified + mtime_modified = self.fs.getinfo("bar", ("modified",)).modified + # Microsecond and seconds might not actually be supported by all + # FTP commands, so we strip them before comparing if it looks + # like at least one of the two values does not contain them. + replacement = {} + if mtime_detail.microsecond == 0 or mtime_modified.microsecond == 0: + replacement["microsecond"] = 0 + if mtime_detail.second == 0 or mtime_modified.second == 0: + replacement["second"] = 0 + self.assertEqual( + mtime_detail.replace(**replacement), mtime_modified.replace(**replacement) + ) + def test_opener_path(self): self.fs.makedir("foo") self.fs.writetext("foo/bar", "baz") @@ -301,7 +317,6 @@ def test_features(self): @mark.slow @unittest.skipIf(platform.python_implementation() == "PyPy", "ftp unreliable with PyPy") class TestAnonFTPFS(FSTestCases, unittest.TestCase): - user = "anonymous" pasw = "" diff --git a/tests/test_memoryfs.py b/tests/test_memoryfs.py index 31909f0f..980b694d 100644 --- a/tests/test_memoryfs.py +++ b/tests/test_memoryfs.py @@ -69,7 +69,6 @@ def test_close_mem_free(self): class TestMemoryFile(unittest.TestCase): - def setUp(self): self.fs = memoryfs.MemoryFS() diff --git a/tests/test_opener.py b/tests/test_opener.py index e7fae983..bc2f5cd7 100644 --- a/tests/test_opener.py +++ b/tests/test_opener.py @@ -300,14 +300,26 @@ def test_user_data_opener(self, app_dir): def test_open_ftp(self, mock_FTPFS): open_fs("ftp://foo:bar@ftp.example.org") mock_FTPFS.assert_called_once_with( - "ftp.example.org", passwd="bar", port=21, user="foo", proxy=None, timeout=10, tls=False + "ftp.example.org", + passwd="bar", + port=21, + user="foo", + proxy=None, + timeout=10, + tls=False, ) @mock.patch("fs.ftpfs.FTPFS") def test_open_ftps(self, mock_FTPFS): open_fs("ftps://foo:bar@ftp.example.org") mock_FTPFS.assert_called_once_with( - "ftp.example.org", passwd="bar", port=21, user="foo", proxy=None, timeout=10, tls=True + "ftp.example.org", + passwd="bar", + port=21, + user="foo", + proxy=None, + timeout=10, + tls=True, ) @mock.patch("fs.ftpfs.FTPFS") diff --git a/tests/test_wrap.py b/tests/test_wrap.py index 89a91187..e11ded4a 100644 --- a/tests/test_wrap.py +++ b/tests/test_wrap.py @@ -177,7 +177,7 @@ def test_scandir(self): ] with mock.patch.object(self.fs, "scandir", wraps=self.fs.scandir) as scandir: self.assertEqual(sorted(self.cached.scandir("/"), key=key), expected) - scandir.assert_has_calls([mock.call('/', namespaces=None, page=None)]) + scandir.assert_has_calls([mock.call("/", namespaces=None, page=None)]) with mock.patch.object(self.fs, "scandir", wraps=self.fs.scandir) as scandir: self.assertEqual(sorted(self.cached.scandir("/"), key=key), expected) scandir.assert_not_called() @@ -187,7 +187,7 @@ def test_isdir(self): self.assertTrue(self.cached.isdir("foo")) self.assertFalse(self.cached.isdir("egg")) # is file self.assertFalse(self.cached.isdir("spam")) # doesn't exist - scandir.assert_has_calls([mock.call('/', namespaces=None, page=None)]) + scandir.assert_has_calls([mock.call("/", namespaces=None, page=None)]) with mock.patch.object(self.fs, "scandir", wraps=self.fs.scandir) as scandir: self.assertTrue(self.cached.isdir("foo")) self.assertFalse(self.cached.isdir("egg")) @@ -199,7 +199,7 @@ def test_isfile(self): self.assertTrue(self.cached.isfile("egg")) self.assertFalse(self.cached.isfile("foo")) # is dir self.assertFalse(self.cached.isfile("spam")) # doesn't exist - scandir.assert_has_calls([mock.call('/', namespaces=None, page=None)]) + scandir.assert_has_calls([mock.call("/", namespaces=None, page=None)]) with mock.patch.object(self.fs, "scandir", wraps=self.fs.scandir) as scandir: self.assertTrue(self.cached.isfile("egg")) self.assertFalse(self.cached.isfile("foo")) @@ -211,7 +211,7 @@ def test_getinfo(self): self.assertEqual(self.cached.getinfo("foo"), self.fs.getinfo("foo")) self.assertEqual(self.cached.getinfo("/"), self.fs.getinfo("/")) self.assertNotFound(self.cached.getinfo, "spam") - scandir.assert_has_calls([mock.call('/', namespaces=None, page=None)]) + scandir.assert_has_calls([mock.call("/", namespaces=None, page=None)]) with mock.patch.object(self.fs, "scandir", wraps=self.fs.scandir) as scandir: self.assertEqual(self.cached.getinfo("foo"), self.fs.getinfo("foo")) self.assertEqual(self.cached.getinfo("/"), self.fs.getinfo("/")) From 29354ef4cf601d9b8d89fbaef84c4f79b4d426e2 Mon Sep 17 00:00:00 2001 From: atollk Date: Thu, 15 Apr 2021 09:28:19 +0200 Subject: [PATCH 2/8] Added support for "last modified time" in `getinfo`. A new namespace "modified" was introduced to the `getinfo` function to potentially use cheaper commands compared to the general "details" namespace. In particular, `FTPFS` now uses the MDTM command if supported by the server. --- CHANGELOG.md | 6 ++++++ fs/base.py | 21 +++++++++++++++++++++ fs/copy.py | 10 ++++------ fs/ftpfs.py | 14 ++++++++++++++ fs/info.py | 9 ++++++--- fs/osfs.py | 1 + tests/test_ftpfs.py | 17 +++++++++++++++++ tests/test_memoryfs.py | 7 +++---- 8 files changed, 72 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c24b0e66..67562281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `fs.copy.copy_file_if`, `fs.copy.copy_dir_if`, and `fs.copy.copy_fs_if`. Closes [#458](https://github.com/PyFilesystem/pyfilesystem2/issues/458). +### Changed + +- FTP servers that do not support the MLST command now try to use the MDTM command to + retrieve the last modification timestamp of a resource. + Closes [#456](https://github.com/PyFilesystem/pyfilesystem2/pull/456). + ### Fixed - Fixed performance bugs in `fs.copy.copy_dir_if_newer`. Test cases were adapted to catch those bugs in the future. diff --git a/fs/base.py b/fs/base.py index 8d8ee4c8..6283b70f 100644 --- a/fs/base.py +++ b/fs/base.py @@ -697,6 +697,25 @@ def readtext( gettext = _new_name(readtext, "gettext") + def getmodified(self, path): + # type: (Text) -> Optional[datetime] + """Get the timestamp of the last modifying access of a resource. + + Arguments: + path (str): A path to a resource. + + Returns: + datetime: The timestamp of the last modification. + + The *modified timestamp* of a file is the point in time + that the file was last changed. Depending on the file system, + it might only have limited accuracy. + + """ + if self.getmeta().get("supports_mtime", False): + return self.getinfo(path, namespaces=["modified"]).modified + return self.getinfo(path, namespaces=["details"]).modified + def getmeta(self, namespace="standard"): # type: (Text) -> Mapping[Text, object] """Get meta information regarding a filesystem. @@ -734,6 +753,8 @@ def getmeta(self, namespace="standard"): read_only `True` if this filesystem is read only. supports_rename `True` if this filesystem supports an `os.rename` operation. + supports_mtime `True` if this filesystem supports a native + operation to retreive the "last modified" time. =================== ============================================ Most builtin filesystems will provide all these keys, and third- diff --git a/fs/copy.py b/fs/copy.py index 95650b94..6ffd83d7 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -463,9 +463,8 @@ def _copy_is_necessary( elif condition == "newer": try: - namespace = ("details",) - src_modified = src_fs.getinfo(src_path, namespace).modified - dst_modified = dst_fs.getinfo(dst_path, namespace).modified + src_modified = src_fs.getmodified(src_path) + dst_modified = dst_fs.getmodified(dst_path) except ResourceNotFound: return True else: @@ -477,9 +476,8 @@ def _copy_is_necessary( elif condition == "older": try: - namespace = ("details",) - src_modified = src_fs.getinfo(src_path, namespace).modified - dst_modified = dst_fs.getinfo(dst_path, namespace).modified + src_modified = src_fs.getmodified(src_path) + dst_modified = dst_fs.getmodified(dst_path) except ResourceNotFound: return True else: diff --git a/fs/ftpfs.py b/fs/ftpfs.py index 86f04958..418d554a 100644 --- a/fs/ftpfs.py +++ b/fs/ftpfs.py @@ -16,6 +16,7 @@ from contextlib import contextmanager from ftplib import FTP + try: from ftplib import FTP_TLS except ImportError as err: @@ -667,6 +668,18 @@ def getinfo(self, path, namespaces=None): } ) + if "modified" in namespaces: + if "basic" in namespaces or "details" in namespaces: + raise ValueError( + 'Cannot use the "modified" namespace in combination with others.' + ) + with self._lock: + with ftp_errors(self, path=path): + cmd = "MDTM " + _encode(self.validatepath(path), self.ftp.encoding) + response = self.ftp.sendcmd(cmd) + modified_info = {"modified": self._parse_ftp_time(response.split()[1])} + return Info({"modified": modified_info}) + if self.supports_mlst: with self._lock: with ftp_errors(self, path=path): @@ -692,6 +705,7 @@ def getmeta(self, namespace="standard"): if namespace == "standard": _meta = self._meta.copy() _meta["unicode_paths"] = "UTF8" in self.features + _meta["supports_mtime"] = "MDTM" in self.features return _meta def listdir(self, path): diff --git a/fs/info.py b/fs/info.py index 03bf27cd..6c6cdb0e 100644 --- a/fs/info.py +++ b/fs/info.py @@ -317,9 +317,12 @@ def modified(self): namespace is not in the Info. """ - self._require_namespace("details") - _time = self._make_datetime(self.get("details", "modified")) - return _time + try: + self._require_namespace("details") + return self._make_datetime(self.get("details", "modified")) + except MissingInfoNamespace: + self._require_namespace("modified") + return self._make_datetime(self.get("modified", "modified")) @property def created(self): diff --git a/fs/osfs.py b/fs/osfs.py index ac43471a..c0de12cd 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -145,6 +145,7 @@ def __init__( "network": False, "read_only": False, "supports_rename": True, + "supports_mtime": False, "thread_safe": True, "unicode_paths": os.path.supports_unicode_filenames, "virtual": False, diff --git a/tests/test_ftpfs.py b/tests/test_ftpfs.py index 43028da7..0501589e 100644 --- a/tests/test_ftpfs.py +++ b/tests/test_ftpfs.py @@ -261,6 +261,23 @@ def test_getmeta_unicode_path(self): del self.fs.features["UTF8"] self.assertFalse(self.fs.getmeta().get("unicode_paths")) + def test_getinfo_modified(self): + self.assertIn("MDTM", self.fs.features) + self.fs.create("bar") + mtime_detail = self.fs.getinfo("bar", ("basic", "details")).modified + mtime_modified = self.fs.getinfo("bar", ("modified",)).modified + # Microsecond and seconds might not actually be supported by all + # FTP commands, so we strip them before comparing if it looks + # like at least one of the two values does not contain them. + replacement = {} + if mtime_detail.microsecond == 0 or mtime_modified.microsecond == 0: + replacement["microsecond"] = 0 + if mtime_detail.second == 0 or mtime_modified.second == 0: + replacement["second"] = 0 + self.assertEqual( + mtime_detail.replace(**replacement), mtime_modified.replace(**replacement) + ) + def test_opener_path(self): self.fs.makedir("foo") self.fs.writetext("foo/bar", "baz") diff --git a/tests/test_memoryfs.py b/tests/test_memoryfs.py index bb5096f9..74cdc2d6 100644 --- a/tests/test_memoryfs.py +++ b/tests/test_memoryfs.py @@ -72,14 +72,13 @@ def test_copy_preserve_time(self): self.fs.makedir("bar") self.fs.touch("foo/file.txt") - namespaces = ("details", "modified") - src_info = self.fs.getinfo("foo/file.txt", namespaces) + src_info = self.fs.getmodified("foo/file.txt") self.fs.copy("foo/file.txt", "bar/file.txt", preserve_time=True) self.assertTrue(self.fs.exists("bar/file.txt")) - dst_info = self.fs.getinfo("bar/file.txt", namespaces) - self.assertEqual(dst_info.modified, src_info.modified) + dst_info = self.fs.getmodified("bar/file.txt") + self.assertEqual(dst_info, src_info) class TestMemoryFile(unittest.TestCase): From 3065813714dc8c2d3221538a5e7218e84e6bda37 Mon Sep 17 00:00:00 2001 From: atollk Date: Tue, 27 Apr 2021 09:41:07 +0200 Subject: [PATCH 3/8] Fixes from code review. --- CHANGELOG.md | 1 + docs/source/interface.rst | 1 + fs/base.py | 2 +- fs/ftpfs.py | 26 +++++++++++++++++--------- tests/test_memoryfs.py | 6 +++--- 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67562281..968dc677 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `fs.copy.copy_file_if`, `fs.copy.copy_dir_if`, and `fs.copy.copy_fs_if`. Closes [#458](https://github.com/PyFilesystem/pyfilesystem2/issues/458). +- Added `fs.base.FS.getmodified`. ### Changed diff --git a/docs/source/interface.rst b/docs/source/interface.rst index e2da135b..0d67c0c6 100644 --- a/docs/source/interface.rst +++ b/docs/source/interface.rst @@ -20,6 +20,7 @@ The following is a complete list of methods on PyFilesystem objects. * :meth:`~fs.base.FS.getdetails` Get details info namespace for a resource. * :meth:`~fs.base.FS.getinfo` Get info regarding a file or directory. * :meth:`~fs.base.FS.getmeta` Get meta information for a resource. +* :meth:`~fs.base.FS.getmodified` Get info regarding the last modified time of a resource. * :meth:`~fs.base.FS.getospath` Get path with encoding expected by the OS. * :meth:`~fs.base.FS.getsize` Get the size of a file. * :meth:`~fs.base.FS.getsyspath` Get the system path of a resource, if one exists. diff --git a/fs/base.py b/fs/base.py index 6283b70f..55edf3a4 100644 --- a/fs/base.py +++ b/fs/base.py @@ -754,7 +754,7 @@ def getmeta(self, namespace="standard"): supports_rename `True` if this filesystem supports an `os.rename` operation. supports_mtime `True` if this filesystem supports a native - operation to retreive the "last modified" time. + operation to retrieve the "last modified" time. =================== ============================================ Most builtin filesystems will provide all these keys, and third- diff --git a/fs/ftpfs.py b/fs/ftpfs.py index 418d554a..260592ce 100644 --- a/fs/ftpfs.py +++ b/fs/ftpfs.py @@ -12,6 +12,7 @@ import socket import threading import typing +import warnings from collections import OrderedDict from contextlib import contextmanager from ftplib import FTP @@ -669,16 +670,23 @@ def getinfo(self, path, namespaces=None): ) if "modified" in namespaces: - if "basic" in namespaces or "details" in namespaces: - raise ValueError( - 'Cannot use the "modified" namespace in combination with others.' + if "details" in namespaces: + warnings.warn( + "FTPFS.getinfo called with both 'modified' and 'details'" + " namespace. The former will be ignored.", + UserWarning, ) - with self._lock: - with ftp_errors(self, path=path): - cmd = "MDTM " + _encode(self.validatepath(path), self.ftp.encoding) - response = self.ftp.sendcmd(cmd) - modified_info = {"modified": self._parse_ftp_time(response.split()[1])} - return Info({"modified": modified_info}) + else: + with self._lock: + with ftp_errors(self, path=path): + cmd = "MDTM " + _encode( + self.validatepath(path), self.ftp.encoding + ) + response = self.ftp.sendcmd(cmd) + modified_info = { + "modified": self._parse_ftp_time(response.split()[1]) + } + return Info({"modified": modified_info}) if self.supports_mlst: with self._lock: diff --git a/tests/test_memoryfs.py b/tests/test_memoryfs.py index 74cdc2d6..67d92ac1 100644 --- a/tests/test_memoryfs.py +++ b/tests/test_memoryfs.py @@ -72,13 +72,13 @@ def test_copy_preserve_time(self): self.fs.makedir("bar") self.fs.touch("foo/file.txt") - src_info = self.fs.getmodified("foo/file.txt") + src_datetime = self.fs.getmodified("foo/file.txt") self.fs.copy("foo/file.txt", "bar/file.txt", preserve_time=True) self.assertTrue(self.fs.exists("bar/file.txt")) - dst_info = self.fs.getmodified("bar/file.txt") - self.assertEqual(dst_info, src_info) + dst_datetime = self.fs.getmodified("bar/file.txt") + self.assertEqual(dst_datetime, src_datetime) class TestMemoryFile(unittest.TestCase): From ee211479c7df1e5297bf929b1c93fab649ea783c Mon Sep 17 00:00:00 2001 From: atollk Date: Thu, 29 Apr 2021 01:08:39 +0200 Subject: [PATCH 4/8] Fixes from code review. --- fs/info.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/fs/info.py b/fs/info.py index 6c6cdb0e..e7a4fd84 100644 --- a/fs/info.py +++ b/fs/info.py @@ -317,12 +317,9 @@ def modified(self): namespace is not in the Info. """ - try: - self._require_namespace("details") - return self._make_datetime(self.get("details", "modified")) - except MissingInfoNamespace: - self._require_namespace("modified") - return self._make_datetime(self.get("modified", "modified")) + namespace = "modified" if self.has_namespace("modified") else "details" + self._require_namespace(namespace) + return self._make_datetime(self.get(namespace, "modified")) @property def created(self): From 145bcea8d9a5572b6e99b6a13e20e75ffc667c9b Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Sun, 27 Jun 2021 14:34:29 +0200 Subject: [PATCH 5/8] Remove the `supports_mtime` meta feature --- fs/base.py | 4 ---- fs/info.py | 6 +++--- fs/osfs.py | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/fs/base.py b/fs/base.py index 55edf3a4..82a5978d 100644 --- a/fs/base.py +++ b/fs/base.py @@ -712,8 +712,6 @@ def getmodified(self, path): it might only have limited accuracy. """ - if self.getmeta().get("supports_mtime", False): - return self.getinfo(path, namespaces=["modified"]).modified return self.getinfo(path, namespaces=["details"]).modified def getmeta(self, namespace="standard"): @@ -753,8 +751,6 @@ def getmeta(self, namespace="standard"): read_only `True` if this filesystem is read only. supports_rename `True` if this filesystem supports an `os.rename` operation. - supports_mtime `True` if this filesystem supports a native - operation to retrieve the "last modified" time. =================== ============================================ Most builtin filesystems will provide all these keys, and third- diff --git a/fs/info.py b/fs/info.py index e7a4fd84..03bf27cd 100644 --- a/fs/info.py +++ b/fs/info.py @@ -317,9 +317,9 @@ def modified(self): namespace is not in the Info. """ - namespace = "modified" if self.has_namespace("modified") else "details" - self._require_namespace(namespace) - return self._make_datetime(self.get(namespace, "modified")) + self._require_namespace("details") + _time = self._make_datetime(self.get("details", "modified")) + return _time @property def created(self): diff --git a/fs/osfs.py b/fs/osfs.py index c0de12cd..ac43471a 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -145,7 +145,6 @@ def __init__( "network": False, "read_only": False, "supports_rename": True, - "supports_mtime": False, "thread_safe": True, "unicode_paths": os.path.supports_unicode_filenames, "virtual": False, From 2d9b6375d6d3958bd27615b8ff83594bf062b40b Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Sun, 27 Jun 2021 14:48:30 +0200 Subject: [PATCH 6/8] Overload `FTPFS.getmodified` to use the MDTM feature if available --- fs/ftpfs.py | 38 +++++++++++++++++++------------------- tests/test_ftpfs.py | 3 ++- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/fs/ftpfs.py b/fs/ftpfs.py index 260592ce..9f480625 100644 --- a/fs/ftpfs.py +++ b/fs/ftpfs.py @@ -42,6 +42,7 @@ from .path import basename from .path import normpath from .path import split +from .time import epoch_to_datetime from . import _ftp_parse as ftp_parse if typing.TYPE_CHECKING: @@ -574,6 +575,12 @@ def supports_mlst(self): """bool: whether the server supports MLST feature.""" return "MLST" in self.features + @property + def supports_mdtm(self): + # type: () -> bool + """bool: whether the server supports the MDTM feature.""" + return "MDTM" in self.features + def create(self, path, wipe=False): # type: (Text, bool) -> bool _path = self.validatepath(path) @@ -669,25 +676,6 @@ def getinfo(self, path, namespaces=None): } ) - if "modified" in namespaces: - if "details" in namespaces: - warnings.warn( - "FTPFS.getinfo called with both 'modified' and 'details'" - " namespace. The former will be ignored.", - UserWarning, - ) - else: - with self._lock: - with ftp_errors(self, path=path): - cmd = "MDTM " + _encode( - self.validatepath(path), self.ftp.encoding - ) - response = self.ftp.sendcmd(cmd) - modified_info = { - "modified": self._parse_ftp_time(response.split()[1]) - } - return Info({"modified": modified_info}) - if self.supports_mlst: with self._lock: with ftp_errors(self, path=path): @@ -716,6 +704,18 @@ def getmeta(self, namespace="standard"): _meta["supports_mtime"] = "MDTM" in self.features return _meta + def getmodified(self, path): + # type: (Text) -> Optional[datetime] + if self.supports_mdtm: + _path = self.validatepath(path) + with self._lock: + with ftp_errors(self, path=path): + cmd = "MDTM " + _encode(_path, self.ftp.encoding) + response = self.ftp.sendcmd(cmd) + mtime = self._parse_ftp_time(response.split()[1]) + return epoch_to_datetime(mtime) + return super().getmodified(self, path) + def listdir(self, path): # type: (Text) -> List[Text] _path = self.validatepath(path) diff --git a/tests/test_ftpfs.py b/tests/test_ftpfs.py index 0501589e..935582ff 100644 --- a/tests/test_ftpfs.py +++ b/tests/test_ftpfs.py @@ -265,7 +265,8 @@ def test_getinfo_modified(self): self.assertIn("MDTM", self.fs.features) self.fs.create("bar") mtime_detail = self.fs.getinfo("bar", ("basic", "details")).modified - mtime_modified = self.fs.getinfo("bar", ("modified",)).modified + mtime_modified = self.fs.getmodified("bar") + print(mtime_detail, mtime_modified) # Microsecond and seconds might not actually be supported by all # FTP commands, so we strip them before comparing if it looks # like at least one of the two values does not contain them. From 5a67c8a1bcd48d6c0153ec4ed58b2a256cf1c162 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Sun, 27 Jun 2021 14:56:30 +0200 Subject: [PATCH 7/8] Fix type annotations and unused imports in `fs.ftpfs` --- fs/ftpfs.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/ftpfs.py b/fs/ftpfs.py index 9f480625..b7a49988 100644 --- a/fs/ftpfs.py +++ b/fs/ftpfs.py @@ -12,12 +12,10 @@ import socket import threading import typing -import warnings from collections import OrderedDict from contextlib import contextmanager from ftplib import FTP - try: from ftplib import FTP_TLS except ImportError as err: @@ -705,7 +703,7 @@ def getmeta(self, namespace="standard"): return _meta def getmodified(self, path): - # type: (Text) -> Optional[datetime] + # type: (Text) -> Optional[datetime.datetime] if self.supports_mdtm: _path = self.validatepath(path) with self._lock: @@ -714,7 +712,7 @@ def getmodified(self, path): response = self.ftp.sendcmd(cmd) mtime = self._parse_ftp_time(response.split()[1]) return epoch_to_datetime(mtime) - return super().getmodified(self, path) + return super(FTPFS, self).getmodified(path) def listdir(self, path): # type: (Text) -> List[Text] From 4feb2425bc220716f0beb2fa313c9ebfc1ec65e1 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Sat, 3 Jul 2021 11:53:59 +0200 Subject: [PATCH 8/8] Remove debug print from `tests.test_ftpfs` --- tests/test_ftpfs.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_ftpfs.py b/tests/test_ftpfs.py index 935582ff..d4143aa0 100644 --- a/tests/test_ftpfs.py +++ b/tests/test_ftpfs.py @@ -266,7 +266,6 @@ def test_getinfo_modified(self): self.fs.create("bar") mtime_detail = self.fs.getinfo("bar", ("basic", "details")).modified mtime_modified = self.fs.getmodified("bar") - print(mtime_detail, mtime_modified) # Microsecond and seconds might not actually be supported by all # FTP commands, so we strip them before comparing if it looks # like at least one of the two values does not contain them.