Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solving bug in rename function #6735

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Solving bug in rename function #6735

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cfd8b31
Solving bug in rename function
Jan 28, 2025
790204e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 28, 2025
d4b2b4d
Applying review on issue 6735
ayushjariyal Jan 29, 2025
a826a4c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 29, 2025
484a9f7
Resolved merge conflict in rename method
ayushjariyal Jan 29, 2025
051ef84
Solve merge conflicts
ayushjariyal Jan 29, 2025
2023dbe
applying review
ayushjariyal Jan 29, 2025
e2c34db
Update docstring
ayushjariyal Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/aiida/transports/plugins/ssh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.isfile(newpath) or self.isdir(newpath):
if self.path_exists(newpath):
raise OSError(f'Destination {newpath} already exists')

maybe this is easier, and perhaps more readable.
Also I'm not entirely sure, if existing symlinks would get caught by self.isfile

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also get rid of the comments above, line 1374-1377

raise OSError(f'Destination {newpath} already exist')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring on line 1360 needs fixing. (github does not allow me to comment on that line)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the docstring on line 1360, it states that OSError is used for both cases—when oldpath and newpath both are not found—which needs to be fixed. However, is it okay to use specific errors (FileNotFoundError and FileExistsError) instead of OSError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

                   :raises OSError: if oldpath is not found

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general. you are right FileNotFoundError might be a better fit for such cases, as with OSError is not immediately clear what's the error case.

However, please note OSError is the parent of FileNotFoundError

 └── Exception
      └── OSError
           ├── FileNotFoundError
           ├── PermissionError
           ├── IsADirectoryError
           ├── NotADirectoryError

For the sake of consistency --I assume-- OSError was chose to raise from our Transport class with proper message. This way it's easier to handle, when adopting any of the Transport plugins.


return self.sftp.rename(oldpath, newpath)

Expand Down