Solving bug in rename function #6735
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks @ayushjariyal for your contribution, please apply the review.
src/aiida/transports/plugins/ssh.py
Outdated
| if not self.isdir(newpath): | ||
| raise OSError(f'Destination {newpath} does not exist') | ||
|
|
||
| if self.isfile(newpath) or self.isdir(newpath): |
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
Please also get rid of the comments above, line 1374-1377
for more information, see https://pre-commit.ci
|
@khsrali , I have made the changes as per your instructions. Could you please review them? |
Please check here https://github.com/aiidateam/aiida-core/pull/6735/files |
|
@khsrali I solved the merge conflict. Can you please check it? |
|
bro, apply the review carefully 🥲 |
| raise OSError(f'Destination {newpath} does not exist') | ||
|
|
||
| if self.path_exists(newpath): | ||
| raise OSError(f'Destination {newpath} already exist') |
There was a problem hiding this comment.
The docstring on line 1360 needs fixing. (github does not allow me to comment on that line)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
:raises OSError: if oldpath is not found
There was a problem hiding this comment.
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.
yes, apparently never used across the interface 🥲 Agreed, adding tests is always a good idea! @ayushjariyal can you please write tests for this function? you can look for file |
Fix #6725
Updated the condition to raise an appropriate error message (Destination already exists) when the path exists, improving code readability and correctness.