ENH: add useful error message when can't change directories on ftp site #189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request improves error handling when the program fails to change directories on an FTP site. It adds a try-except block to catch the error and prints a user-friendly error message to the console, including potential reasons for the failure. Sequence diagram for enhanced FTP directory listing error handlingsequenceDiagram
participant Client
participant ListDir
participant FTP
Client->>ListDir: listdir(path)
ListDir->>FTP: configured_ftp()
activate FTP
FTP-->>ListDir: ftp connection
deactivate FTP
ListDir->>FTP: cwd(path)
alt Success
FTP-->>ListDir: directory changed
ListDir->>FTP: nlst()
FTP-->>ListDir: file list
ListDir-->>Client: yield files
else error_perm
FTP-->>ListDir: permission error
ListDir->>ListDir: print error message
ListDir-->>Client: return None
end
ListDir->>FTP: close()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GavinHuttley - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+26
to
+35
| try: | ||
| ftp.cwd(path) | ||
| except error_perm: | ||
| msg = "\n".join( | ||
| [ | ||
| "", | ||
| f"Skipping {path}", | ||
| "Could not change into directory on FTP site.", | ||
| "This can be because of Ensembl naming inconsistencies.", | ||
| "Check names are consistent between genomes and homology.", |
There was a problem hiding this comment.
🚨 suggestion (security): Consider using a context manager or try-finally block to ensure FTP connection is always closed
The current implementation might leak FTP connections if errors occur. Using 'with' statement or try-finally would guarantee proper cleanup.
Suggested implementation:
from contextlib import contextmanager
from ftplib import FTP, error_perm """returns directory listing"""
pattern = pattern or (lambda x: True)
with configured_ftp_cm(host=host) as ftp:
try:
ftp.cwd(path)
except error_perm:
msg = "\n".join(
[
"",
f"Skipping {path}",
"Could not change into directory on FTP site.",
"This can be because of Ensembl naming inconsistencies.",
"Check names are consistent between genomes and homology.",
],
)
eti_util.print_colour(msg, "red")
return
for fn in ftp.nlst():
if pattern(fn):
yield f"{path}/{fn}"
@contextmanager
def configured_ftp_cm(host: str):
"""Context manager for FTP connections"""
ftp = configured_ftp(host=host)
try:
yield ftp
finally:
ftp.close()| ) | ||
| eti_util.print_colour(msg, "red") | ||
| return | ||
|
|
||
| for fn in ftp.nlst(): |
There was a problem hiding this comment.
issue: Add error handling for potential FTP failures during directory listing
The nlst() call could fail if the connection is lost after changing directory. Consider wrapping it in a try-except block.
Pull Request Test Coverage Report for Build 13146911781Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Bug Fixes:
error_permwhen changing directories on the FTP site and print a more informative error message if it occurs.