From cfd8b313c3a2b8b25dc84182e1e663f7caccb5d2 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Tue, 28 Jan 2025 15:47:49 +0530 Subject: [PATCH 01/15] Solving bug in rename function --- src/aiida/transports/plugins/ssh.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index f5d0e7053f..9f98272598 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1375,9 +1375,9 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): # why to raise an OSError if the newpath does not exist? # ofcourse newpath shouldn't exist, that's why we are renaming it! # issue opened here: https://github.com/aiidateam/aiida-core/issues/6725 - if not self.isfile(newpath): - if not self.isdir(newpath): - raise OSError(f'Destination {newpath} does not exist') + + if self.isfile(newpath) or self.isdir(newpath): + raise OSError(f"Destination {newpath} already exist") return self.sftp.rename(oldpath, newpath) From 790204e2ac954f5ff4408b7d5e62ec10dce9a054 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 28 Jan 2025 18:36:09 +0000 Subject: [PATCH 02/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/transports/plugins/ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index 9f98272598..47390f123e 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1375,9 +1375,9 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): # why to raise an OSError if the newpath does not exist? # ofcourse newpath shouldn't exist, that's why we are renaming it! # issue opened here: https://github.com/aiidateam/aiida-core/issues/6725 - + if self.isfile(newpath) or self.isdir(newpath): - raise OSError(f"Destination {newpath} already exist") + raise OSError(f'Destination {newpath} already exist') return self.sftp.rename(oldpath, newpath) From d4b2b4dec075a406028e6cc6a89cafd8dcadcc79 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Wed, 29 Jan 2025 15:23:11 +0530 Subject: [PATCH 03/15] Applying review on issue 6735 --- src/aiida/transports/plugins/ssh.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index 47390f123e..45feeb133d 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1371,6 +1371,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): if not self.isfile(oldpath): if not self.isdir(oldpath): raise OSError(f'Source {oldpath} does not exist') +<<<<<<< HEAD # TODO: this seems to be a bug (?) # why to raise an OSError if the newpath does not exist? # ofcourse newpath shouldn't exist, that's why we are renaming it! @@ -1378,6 +1379,11 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): if self.isfile(newpath) or self.isdir(newpath): raise OSError(f'Destination {newpath} already exist') +======= + + if self.path_exists(newpath): + raise OSError(f"Destination {newpath} already exist") +>>>>>>> 5ecb0476b (Appling review on issue #6735) return self.sftp.rename(oldpath, newpath) From a826a4c680771578ec2f59e6c85c5c553ea529e1 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:54:43 +0000 Subject: [PATCH 04/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/transports/plugins/ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index 45feeb133d..55ca13f9a4 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1380,7 +1380,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): if self.isfile(newpath) or self.isdir(newpath): raise OSError(f'Destination {newpath} already exist') ======= - + if self.path_exists(newpath): raise OSError(f"Destination {newpath} already exist") >>>>>>> 5ecb0476b (Appling review on issue #6735) From 484a9f79f3862c77fdf6f0425b5acfa78ca66670 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Wed, 29 Jan 2025 15:55:19 +0530 Subject: [PATCH 05/15] Resolved merge conflict in rename method --- src/aiida/transports/plugins/ssh.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index 45feeb133d..cbe2348b66 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1371,19 +1371,9 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): if not self.isfile(oldpath): if not self.isdir(oldpath): raise OSError(f'Source {oldpath} does not exist') -<<<<<<< HEAD - # TODO: this seems to be a bug (?) - # why to raise an OSError if the newpath does not exist? - # ofcourse newpath shouldn't exist, that's why we are renaming it! - # issue opened here: https://github.com/aiidateam/aiida-core/issues/6725 - - if self.isfile(newpath) or self.isdir(newpath): - raise OSError(f'Destination {newpath} already exist') -======= - + if self.path_exists(newpath): raise OSError(f"Destination {newpath} already exist") ->>>>>>> 5ecb0476b (Appling review on issue #6735) return self.sftp.rename(oldpath, newpath) From 2023dbe27e05b0d3c7b2932388cf13289afe10e3 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Wed, 29 Jan 2025 22:08:26 +0530 Subject: [PATCH 06/15] applying review --- src/aiida/transports/plugins/ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index ab861b4970..e6b85eca8c 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1372,7 +1372,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): if not self.isdir(oldpath): raise OSError(f'Source {oldpath} does not exist') - if self.isfile(newpath) or self.isdir(newpath): + if self.path_exists(newpath): raise OSError(f'Destination {newpath} already exist') return self.sftp.rename(oldpath, newpath) From e2c34db667b3e04fe139e0c57da30d4f25812533 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Thu, 30 Jan 2025 14:17:25 +0530 Subject: [PATCH 07/15] Update docstring --- src/aiida/transports/plugins/ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index e6b85eca8c..c67ee1a74c 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1357,7 +1357,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): :param str oldpath: existing name of the file or folder :param str newpath: new name for the file or folder - :raises OSError: if oldpath/newpath is not found + :raises OSError: if oldpath is not found :raises ValueError: if sroldpathc/newpath is not a valid path """ if not oldpath: From ba66b33dfd2045356f64f5d67c6d1f573fbb67d3 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Fri, 7 Feb 2025 19:33:38 +0530 Subject: [PATCH 08/15] Adding test_rename function --- src/aiida/transports/plugins/local.py | 9 +++++-- tests/transports/test_all_plugins.py | 35 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/aiida/transports/plugins/local.py b/src/aiida/transports/plugins/local.py index 937c499861..73d98efe4a 100644 --- a/src/aiida/transports/plugins/local.py +++ b/src/aiida/transports/plugins/local.py @@ -877,8 +877,13 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): raise ValueError(f'Destination {newpath} is not a valid string') if not os.path.exists(oldpath): raise OSError(f'Source {oldpath} does not exist') - if not os.path.exists(newpath): - raise OSError(f'Destination {newpath} does not exist') + # if not os.path.exists(newpath): + # raise OSError(f'Destination {newpath} does not exist') + + # Ensure the destination folder exists + dest_dir = os.path.dirname(newpath) + if dest_dir and not os.path.exists(dest_dir): + raise OSError(f'Destination directory {dest_dir} does not exist') shutil.move(oldpath, newpath) diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 1dcd9c6957..8bb94fb68c 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1232,3 +1232,38 @@ def test_asynchronous_execution(custom_transport, tmp_path): except ProcessLookupError: # If the process is already dead (or has never run), I just ignore the error pass + + +def test_rename(custom_transport, tmp_path_remote): + """Test the rename function of the transport plugin.""" + with custom_transport as transport: + if not transport.is_open: + transport.open() + old_file = tmp_path_remote / 'oldfile.txt' + new_file = tmp_path_remote / 'newfile.txt' + another_file = tmp_path_remote / 'anotherfile.txt' + + # Create a test file to rename + old_file.touch() + another_file.touch() + assert old_file.exists() + assert another_file.exists() + + # Perform rename operation + transport.rename(old_file, new_file) + + # Verify rename was successful + assert not old_file.exists() + assert new_file.exists() + + # Perform rename operation if new file already exists + try: + transport.rename(new_file, another_file) + print('Rename to existing file succeeded (no error).') + except OSError as e: + print(f'Rename failed as expected: {e}') + finally: + # Cleanup files manually + for file in tmp_path_remote.iterdir(): + file.unlink() + tmp_path_remote.rmdir() # Remove the directory if empty From 6b6746e5f90a5ac6741575f2cf0641b33075f947 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Fri, 7 Feb 2025 19:41:17 +0530 Subject: [PATCH 09/15] Adding test_rename function --- src/aiida/transports/plugins/local.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/transports/plugins/local.py b/src/aiida/transports/plugins/local.py index 73d98efe4a..dac187668d 100644 --- a/src/aiida/transports/plugins/local.py +++ b/src/aiida/transports/plugins/local.py @@ -880,7 +880,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): # if not os.path.exists(newpath): # raise OSError(f'Destination {newpath} does not exist') - # Ensure the destination folder exists + # Ensure the destination folder exists dest_dir = os.path.dirname(newpath) if dest_dir and not os.path.exists(dest_dir): raise OSError(f'Destination directory {dest_dir} does not exist') From 7fffa135adc4630c22f15b7c341de4c4efb5e330 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Sat, 8 Feb 2025 00:08:50 +0530 Subject: [PATCH 10/15] applying review --- src/aiida/transports/plugins/ssh.py | 4 ++-- tests/transports/test_all_plugins.py | 14 ++------------ 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index c67ee1a74c..fb5d7dec79 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -1357,8 +1357,8 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): :param str oldpath: existing name of the file or folder :param str newpath: new name for the file or folder - :raises OSError: if oldpath is not found - :raises ValueError: if sroldpathc/newpath is not a valid path + :raises OSError: if oldpath is not found or newpath already exists + :raises ValueError: if oldpath/newpath is not a valid path """ if not oldpath: raise ValueError(f'Source {oldpath} is not a valid path') diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 8bb94fb68c..7c309f8bc7 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1237,8 +1237,6 @@ def test_asynchronous_execution(custom_transport, tmp_path): def test_rename(custom_transport, tmp_path_remote): """Test the rename function of the transport plugin.""" with custom_transport as transport: - if not transport.is_open: - transport.open() old_file = tmp_path_remote / 'oldfile.txt' new_file = tmp_path_remote / 'newfile.txt' another_file = tmp_path_remote / 'anotherfile.txt' @@ -1257,13 +1255,5 @@ def test_rename(custom_transport, tmp_path_remote): assert new_file.exists() # Perform rename operation if new file already exists - try: - transport.rename(new_file, another_file) - print('Rename to existing file succeeded (no error).') - except OSError as e: - print(f'Rename failed as expected: {e}') - finally: - # Cleanup files manually - for file in tmp_path_remote.iterdir(): - file.unlink() - tmp_path_remote.rmdir() # Remove the directory if empty + with pytest.raises(OSError): + transport.rename(new_file, another_file) \ No newline at end of file From 778148d5dd249541aa2d5a2824d181da65f0fd5d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 7 Feb 2025 18:40:21 +0000 Subject: [PATCH 11/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/transports/test_all_plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 7c309f8bc7..93049b56b5 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1256,4 +1256,4 @@ def test_rename(custom_transport, tmp_path_remote): # Perform rename operation if new file already exists with pytest.raises(OSError): - transport.rename(new_file, another_file) \ No newline at end of file + transport.rename(new_file, another_file) From b7224750eccbd446eef206ee3a0b72967597210f Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Sun, 9 Feb 2025 16:41:45 +0530 Subject: [PATCH 12/15] Applying review --- src/aiida/transports/plugins/local.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/aiida/transports/plugins/local.py b/src/aiida/transports/plugins/local.py index dac187668d..c0915cf84b 100644 --- a/src/aiida/transports/plugins/local.py +++ b/src/aiida/transports/plugins/local.py @@ -866,7 +866,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): :param str oldpath: existing name of the file or folder :param str newpath: new name for the file or folder - :raises OSError: if src/dst is not found + :raises OSError: if oldpath is not found or newpath already exists :raises ValueError: if src/dst is not a valid string """ oldpath = str(oldpath) @@ -877,13 +877,8 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath): raise ValueError(f'Destination {newpath} is not a valid string') if not os.path.exists(oldpath): raise OSError(f'Source {oldpath} does not exist') - # if not os.path.exists(newpath): - # raise OSError(f'Destination {newpath} does not exist') - - # Ensure the destination folder exists - dest_dir = os.path.dirname(newpath) - if dest_dir and not os.path.exists(dest_dir): - raise OSError(f'Destination directory {dest_dir} does not exist') + if os.path.exists(newpath): + raise OSError(f'Destination {newpath} already exists.') shutil.move(oldpath, newpath) From cda46d02dcae7ffc1b894dfd005188e3b7850cd0 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Sun, 9 Feb 2025 16:50:24 +0530 Subject: [PATCH 13/15] applying review --- tests/transports/test_all_plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 93049b56b5..44cdb55205 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1255,5 +1255,5 @@ def test_rename(custom_transport, tmp_path_remote): assert new_file.exists() # Perform rename operation if new file already exists - with pytest.raises(OSError): + with pytest.raises(OSError, match = "already exist"): transport.rename(new_file, another_file) From 35372aede457937c6d89546046a8fa5f878307b5 Mon Sep 17 00:00:00 2001 From: ayushjariyal Date: Sun, 9 Feb 2025 23:44:19 +0530 Subject: [PATCH 14/15] fix test_rename function --- tests/transports/test_all_plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 44cdb55205..587e591558 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1255,5 +1255,5 @@ def test_rename(custom_transport, tmp_path_remote): assert new_file.exists() # Perform rename operation if new file already exists - with pytest.raises(OSError, match = "already exist"): + with pytest.raises(OSError, match = "already exist|destination exists"): transport.rename(new_file, another_file) From 383defc32b0e96b49a3aed1bf20d6c76431d8459 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 9 Feb 2025 18:14:59 +0000 Subject: [PATCH 15/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/transports/test_all_plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index 587e591558..805058ce6d 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1255,5 +1255,5 @@ def test_rename(custom_transport, tmp_path_remote): assert new_file.exists() # Perform rename operation if new file already exists - with pytest.raises(OSError, match = "already exist|destination exists"): + with pytest.raises(OSError, match='already exist|destination exists'): transport.rename(new_file, another_file)