Skip to content

Add FileDownloader class with file:// protocol#430

Open
gyger wants to merge 7 commits intofatiando:mainfrom
gyger:file_copy
Open

Add FileDownloader class with file:// protocol#430
gyger wants to merge 7 commits intofatiando:mainfrom
gyger:file_copy

Conversation

@gyger
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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.

@elphick
Copy link
Copy Markdown

elphick commented Apr 3, 2025

I'm eagerly awaiting this PR merge - fantastic idea. I'm happy to help if needed for those final steps, but will await your thoughts @gyger, @santisoler.

@gyger
Copy link
Copy Markdown
Author

gyger commented Apr 3, 2025

Please go ahead modifying the remaining open points. Was occupied with other stuff.

@elphick
Copy link
Copy Markdown

elphick commented Apr 11, 2025

OK, so I've forked and checked out the gyger/file_copy branch. My branch is called file_copy-finalise. Will chip away over the next week or so and submit a PR.

@elphick
Copy link
Copy Markdown

elphick commented Apr 16, 2025

OK, so the final items are done, but just checking in first to potentially avoid creating a separate PR.

I merged the origin to get up to date before starting work, then:

  • Unit tests using pytest's tmp_path fixture with near complete coverage of the new code
  • Added the new class to the api/index.rst, under the Downloaders section
  • Imported the new class in pooch/init.py
  • Added progressbar functionality
  • Some doc updates, added section to 'Printing progress bars' page

I guess there are two paths forward to merge:

  1. @gyger merges my branch into theirs to maintain this PR, or
  2. I create a separate PR.

Seems to me option 1 would be good, but which would you prefer @gyger?

@elphick
Copy link
Copy Markdown

elphick commented May 28, 2025

Hi @santisoler,
Do you see any issues with this PR now? Is a merge possible?

@santisoler
Copy link
Copy Markdown
Member

Hi @elphick. Sorry for the (very) delayed reply. Life got in the way 😬.

I just pushed a few commits that will make the style checks pass. I'll try to come back to this PR soon.

If you want to continue working on it, please check which checks and tests might be failing and try to address them.

Thanks again for the contribution!

@elphick
Copy link
Copy Markdown

elphick commented Aug 31, 2025

Thanks @santisoler - no need for an apology - I know what that's like. I've not yet come across style conformance yet, so please bear with me - will try to learn more about that.

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.

3 participants