-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
60 handle errors from non http url sources #61
Conversation
Hi Mattia, |
I'll take a look tomorrow (bank holiday in Italy today). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @EddyCMWF,
I have a few comments. I'm approving so you can merge when you are happy with it.
About the tests: As I mentioned in the comments, I'm a bit worried that the library used in this PR is not stable/reliable.
Also, IMO it's better if we don't rely on external files. That way we don't need internet access to run the tests, we don't download external files every time tests are executed, and we are not affected by the external data (e.g., urls could change, downtimes, ...)
Given that the ftp fixture is just about 10 lines of code, I've aligned another branch to this PR in case you change your mind and you want to use my version of the tests: #66
ftp is not compatible with multiurl, so if ftp we use the wget downloader.
Hopefully the approach is generic to facilitate other downloaders in the future.