Conversation
There was a problem hiding this comment.
Pull request overview
Fixes HpGFZ data downloading by switching from an HTTP-based downloader to FTP, aligning the implementation with GFZ’s FTP endpoint.
Changes:
- Replace
requests.get(...)download logic with anftplib.FTP-based implementation inHpGFZ._download. - Update HpGFZ download unit test to mock FTP interactions instead of HTTP requests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
swvo/io/hp/gfz.py |
Implements FTP download logic for HpGFZ files (replacing the previous HTTP approach). |
tests/io/hp/test_hp.py |
Updates the download/process test to mock FTP calls and assert expected FTP operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mock_ftp.retrbinary = mocker.Mock() | ||
| mocker.patch("swvo.io.hp.gfz.FTP", return_value=mock_ftp) | ||
|
|
||
| mocker.patch("shutil.rmtree") |
There was a problem hiding this comment.
This patch won’t intercept the cleanup call: gfz.py imports rmtree as from shutil import rmtree and calls it as rmtree(...). Patch swvo.io.hp.gfz.rmtree instead of shutil.rmtree so the test reliably avoids deleting the temp directory.
| mocker.patch("shutil.rmtree") | |
| mocker.patch("swvo.io.hp.gfz.rmtree") |
| ftp = FTP("ftp.gfz-potsdam.de") | ||
| ftp.login() | ||
| ftp.cwd("/pub/home/obs/Hpo/") | ||
|
|
||
| with open(local_path, "wb") as f: | ||
| ftp.retrbinary(f"RETR {filename}", f.write) | ||
|
|
||
| ftp.quit() |
There was a problem hiding this comment.
ftp.quit() is only called on the success path. If login(), cwd(), or retrbinary() raises, the FTP connection can remain open. Use a context manager (with FTP(...) as ftp:) or a finally block that calls ftp.close()/ftp.quit() when the connection was created.
| ftp = FTP("ftp.gfz-potsdam.de") | |
| ftp.login() | |
| ftp.cwd("/pub/home/obs/Hpo/") | |
| with open(local_path, "wb") as f: | |
| ftp.retrbinary(f"RETR {filename}", f.write) | |
| ftp.quit() | |
| with FTP("ftp.gfz-potsdam.de") as ftp: | |
| ftp.login() | |
| ftp.cwd("/pub/home/obs/Hpo/") | |
| with open(local_path, "wb") as f: | |
| ftp.retrbinary(f"RETR {filename}", f.write) |
| except Exception as e: | ||
| logger.error(f"FTP download failed: {e}") | ||
| raise |
There was a problem hiding this comment.
The except Exception as e: logger.error(...) pattern drops the traceback, which makes diagnosing download failures harder. Prefer logger.exception("FTP download failed") (or pass exc_info=True) and consider catching ftplib.all_errors instead of a bare Exception.
| ftp = FTP("ftp.gfz-potsdam.de") | ||
| ftp.login() | ||
| ftp.cwd("/pub/home/obs/Hpo/") | ||
|
|
There was a problem hiding this comment.
The FTP host and base directory are hard-coded even though URL = "ftp://ftp.gfz-potsdam.de/pub/home/obs/Hpo/" already exists on the class. Consider deriving the host/path from self.URL (or splitting it into FTP_HOST/FTP_BASEDIR constants) to avoid future drift between the constant and the actual download logic.
No description provided.