Add FTP downloader

[andersy005]

This PR adds support for downloading files over FTP. The downloader uses the ftplib module from the standard library.

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst.
• Write detailed docstrings for all functions/methods.
• If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

[welcome]

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

• Remember to run make format to make sure your code follows our style guide.
• If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
• All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
• No matter what, we are really grateful that you put in the effort to do this! 🎉

[leouieda]

@andersy005 thank you for your contribution! It's much appreciated!

[leouieda]

@andersy005 thanks for taking the time to do this! I have some comments on the code and a few things that need to be done before merging.

My main concern here is with adding requests-ftp as a dependency. From their README:

Many corners have been cut in my rush to get this code finished. The most obvious problem is that this code does not have any tests. This is my highest priority for fixing.

Also, there haven't been any commits to the repository since 2017. So making this a mandatory dependency seems risky. Instead, I'd rather make it optional. This can be done the same way we do with tqdm. Then the user can decide if they want to take requests-ftp on as a dependency. Remember that if we add it to Pooch, projects relying on Pooch will automatically have it as a dependency too.

Would you mind making these changes? Let me know if you need any help or orientation.

[andersy005]

My main concern here is with adding requests-ftp as a dependency

@leouieda, I had the same concern as well. I think that it would be straightforward to use the FTP protocol client from the ftplib module instead of request-ftp. By using requests-ftp, I was just being lazy :)

Let me know what your thoughts on using the ftplib module are, and I will update the PR (and will address the changes you requested).

[leouieda]

By using requests-ftp, I was just being lazy :)

I completely understand 😆

Let me know what your thoughts on using the ftplib module are, and I will update the PR (and will address the changes you requested).

That would actually be great! One less dependency and it will be easier to test. I hope it's not a lot of work for you. Too bad it messes up that nice class inheritance...

[leouieda]

My speculation is that there is some strange network translation going on inside TravisCI and I don't know how to address it yet.

Yep, Travis doesn't allow FTP connections: https://docs.travis-ci.com/user/common-build-problems/?utm_source=blog&utm_medium=web&utm_campaign=ftp_blog#ftpsmtpother-protocol-do-not-work

I see you've disabled the FTP tests in Travis, which is what I would recommend. The coverage is reported as low because Azure wasn't uploading coverage for PRs. I just applied a fix to Azure and lets see if it works.

A few more comments on the code. Thanks for taking the time to rewrite things with ftplib! 👍

[leouieda]

@andersy005 please merge in the changes from the master branch. You might have to reformat some docstrings following #123.

[leouieda]

@andersy005 this is looking good! There are some conflicts because of #123 but I'll resolve those locally and push to your branch. Thanks for making all these changes! I really appreciate it.

[leouieda]

👋 @andersy005 I made a few modifications to the code:

• Added a progress bar test
• Refactored the code to simplify it a bit and add some try: ... finally: blocks
• Added a section to the usage docs

Please see what you think and if you're OK with the changes, I'd be happy to merge this in 🙂 Feel free to make any changes you find necessary.

[andersy005]

@leouieda,

Thank you for your help and review! Glad to see this brought to completion. I took a look at the changes you made, and everything looks good to me! Feel free to merge this at your earliest convenience!

[welcome]

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

[leouieda]

Thank you for your contribution @andersy005! Please add yourself to our AUTHORS.md file.

We're also in the midst of writing a paper for JOSS (#112) and with this contribution you are eligible for authorship (see the criteria in #112). It would be a pleasure to have you on board!

[andersy005]

Please add yourself to our AUTHORS.md file.

Done in #125

We're also in the midst of writing a paper for JOSS (#112) and with this contribution you are eligible for authorship (see the criteria in #112). It would be a pleasure to have you on board!

Awesome :) Thank you! See #112 (comment)