Skip to content
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

Add FileDownloader class with file:// protocol #430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gyger
Copy link

@gyger gyger commented Jul 2, 2024

Add FileDownloader class with file protocol to allows repository to be e.g. on a network share

Path needs to start with file://.

Relevant issues/PRs:
See #429

… e.g. on a Network share.

Path needs to start with file://.
@santisoler
Copy link
Member

Hi @gyger. Thanks for opening this PR. Sorry for the delayed reply, I spent last week at a conference.

I'll take a look at this soon.

@santisoler santisoler self-requested a review November 26, 2024 00:03
@santisoler
Copy link
Member

Hi @gyger! This new class looks good! I think it would be a nice addition to Pooch.

A few things that we need to add before we can merge this PR:

  • Unit tests. We would need to add some tests that checks that the downloader works as expected. We could use pytest's tmp_path to create temporary locations from where files will be downloaded from.
  • Add the new class to the api/index.rst, under the Downloaders section.
  • Import the new class in pooch/__init__.py, so it becomes available at the top module level.

It would also be nice to include an example in the documentation on use cases for this new downloader.

Let me know if you can and are willing to address these points. Feel free to ask for help if you need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants