retrieve files from icpac's ftp server #27
Conversation
There was a problem hiding this comment.
Put some comments in the code, it looks good! My main other thoughts are:
- The functions should clearly document the environment variables expected for passing in. (and I think both functions should expect env variables, not mixing up passed in arguments vs env variables). That documentation should probably link to more extensive documentation about ICPAC and how to access/setup these environment variables.
- We should also just have a library-wide documentation on env variables, which I think should encourage use of
.envfiles but have checking that only ensures the variables are set, not checking how (can we ensure there's no error if a.envfile doesn't exist)
|
|
||
|
|
||
| def connect_icpac_ftp( | ||
| ftp_address: str, |
There was a problem hiding this comment.
I would not explicitly pass these variables to connect_icpac_ftp(). I think the docstring should clarify that there are 3 expected environment variables and all the checks for the variables should be performed within this function, rather than retrieve_file_ftp(). Having env variables used for one function and explicit variables in another makes it a confusing API I think.
| ... output_filepath='example.nc') | ||
| """ | ||
| load_dotenv() | ||
| ftp_address = os.getenv("ICPAC_FTP_ADDRESS") |
There was a problem hiding this comment.
As above, I would move this to connect_icpac_ftp().
| """ | ||
| load_dotenv() | ||
| ftp_address = os.getenv("ICPAC_FTP_ADDRESS") | ||
| ftp_username = os.getenv("ICPAC_FTP_USERNAME") |
There was a problem hiding this comment.
I think using a list could be easier and make for more informative error messaging. Maybe something like this?
env_vars = ["ICPAC_FTP_ADDRESS", "ICPAC_FTP_USERNAME", "ICPACP_FTP_PASSWORD"]
ftp_config = [os.getenv(x) for x in env_vars]
ftp_missing = [env_vars[i] for i,e in enumerate(ftp_config) if e is None]
if not ftp_missing:
raise RuntimeError(
f"FTP environment variable(s) missing: {', '.join(ftp_missing)}"
)| if None in (ftp_address, ftp_username, ftp_password): | ||
| raise RuntimeError("One of the ftp variables is not set") | ||
| # TODO: ugly, is there a better method? if not doing, mypy complains | ||
| assert ftp_address is not None, ftp_address |
There was a problem hiding this comment.
Since we are already running the check above, I think explicitly ignoring the mypy error in this case is acceptable I think? For instance, this SO post recommends using assert or a check if it's missing, which has happened, just might be that mypy doesn't detect that, so safe to ignore error I think rather than duplicating with the assert.
There was a problem hiding this comment.
Oh so I actually stopped running mypy from running on the tests in the COD AB PR, there were some super annoying issues with pytest fixtures and I thought it's probably not needed on the tests anyway. I should have flagged it more explicitly though, sorry guys. If you think we should put it back I will try to sort ou the fixture issues!
| ... 'PredictedRainfallProbbability-FMA2021_Jan2021.nc', | ||
| ... output_filepath='example.nc') | ||
| """ | ||
| load_dotenv() |
There was a problem hiding this comment.
Should we be using find_dotenv() as well here as per this SO post? Where does load_dotenv() on its own expect to find the file?
There was a problem hiding this comment.
Reading the documentation of load_dotenv(), there is no warning if the file isn't found by default, so might want to be careful here. However, by default, the code also shows that find_dotenv() is used, so hopefully shouldn't be too problematic?
.env.examplewith.dotenvLeaving at as a draft because
.envretrieve_file_ftpif you know a better method to check for not None and make mypy happy?