customizable callback when download completed

[dreamflasher]

Closes #123

[Cadair]

On a higher level, I am wondering what the advantage to this pattern vs inspecting the returned Results object is. This adds another reasonable size cunk to our user facing API and I am not clear what functionality it's adding.

[dreamflasher]

On a higher level, I am wondering what the advantage to this pattern vs inspecting the returned Results object is. This adds another reasonable size cunk to our user facing API and I am not clear what functionality it's adding.

Thank you for asking! Inspecting the Results object is only possible if it runs until the expection – so all downloads need to be fully processed. If the process is killed meanwhile, that won't happen. But it's your call if you find it useful enough. If you do, I'll make the changes you requested – I agree with them :)

[Cadair]

If the process is killed meanwhile, that won't happen.

That's a fair point, and I guess in an async use case you might end up wanting to do some kind of work after each file is downloaded. I would be happy to add this. I would want to understand and test what happens to the callback if the download is cancelled part way through. Do we always execute the callback or do we cancel that as well?

[dreamflasher]

Do we always execute the callback or do we cancel that as well?

Well, we should think about how we want to design it. Currently, I implemented it that the callback is always called – and in case the download was cancelled we call the callback with the Error. Then you have the flexibility when you implement the callback, to just ignore the error cases, or handle them (do cleanup etc.).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.54%. Comparing base (3b049c5) to head (ea1ca05).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   90.23%   90.54%   +0.30%     
==========================================
  Files           5        5              
  Lines         635      645      +10     
==========================================
+ Hits          573      584      +11     
+ Misses         62       61       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Cadair]

I think we probably want the callbacks to execute even as we are cleaning up the call stack. No idea how we go about testing that though.

[dreamflasher]

@Cadair I added a test for the callback, I use Python 3.11 and locally it works fine – also in your test suite. I don't understand the error that's thrown for py38-conda (why does it work fine for py38?). Would you be so kind as to have a look?

I also think the callback should be called when cleaning up the call stack… but yes, I also don't know how to test this.

[Cadair]

I have a couple of busy weeks of work coming up but I will try and get back to this soon.

[dreamflasher]

@Cadair there doesn't seem to exist a simple way to raise an exception after calling a function?

[Cadair]

I am not sure I follow what you are asking, where do you want to raise the exception?

[Cadair]

This test: https://github.com/Cadair/parfive/blob/main/parfive/tests/test_downloader.py#L324 and specifically the fixture it uses: https://github.com/Cadair/parfive/blob/main/parfive/conftest.py#L9 is rigged up to throw an error every second download. You should be able to reuse that to test that your callback gets passed an exception if a download fails.

[dreamflasher]

@Cadair Thank you! Added the test. Do you have any idea why the build is not working?

[Cadair]

The publish build is failing because of an unrelated Python 3.12 issue.

[dreamflasher]

I guess then this is ready to be merged? :)

[Cadair]

Thanks so much for coming back to this, I think it's very close.

I think using a nonlocal scoped variable will be nicer than the file, if that doesn't work and you need to use a temp file, please put it in tmpdir so it doesn't clutter up the root directory.

[dreamflasher]

nonlocal didn't work, so I went with temp files

[dreamflasher]

@Cadair would you be down for merging it?

[Cadair]

Thanks a lot @dreamflasher 😄