Refactor scraper to exit properly when exceptions are raised

[dan-niles]

Fix #285

Changes:

• Return 1 whenever a exception is raised.
• Move self.zim_file.finish() to an else section in the try-exception block.
• Move the removal of temp directory and files into the finally section in the try-exception block.
• Create delete_callback in utils.py since the one in zimscraperlib.filesystem does not check for a file's existence before trying to delete it.
  • Whenever an exception is raised there could be callbacks that were already initiated. Since the temp folder gets deleted now, these callbacks would throw an error saying FileNotFound. Checking for the existence of the files let's us avoid these errors.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 54 lines in your changes missing coverage. Please review.

Project coverage is 1.53%. Comparing base (60b85b5) to head (d4e3386).

Files	Patch %	Lines
scraper/src/youtube2zim/scraper.py	0.00%	50 Missing ⚠️
scraper/src/youtube2zim/utils.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #288      +/-   ##
========================================
- Coverage   1.54%   1.53%   -0.01%     
========================================
  Files         11      11              
  Lines       1102    1105       +3     
  Branches     162     164       +2     
========================================
  Hits          17      17              
- Misses      1085    1088       +3     

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

[benoit74]

I think this code makes much more sense. I did not reproduced the problem you had on your tests you showed me.

My only concern is that this change shows that we do not delete the build_dir should a problem occur somewhere at the beginning of the function... which is a bit sad.

I suggest we encapsulate the whole method in the try block.

Since we do not call finish when an exception occurs, we can get rid of self.zim_file.can_finish = False statements, so that we do not need to take care of whether self.zim_file has been initialized or not.

I let you test this as well but on my machine it works as expected

[dan-niles]

I moved all the code inside the run method into the try block and removed the self.zim_file.can_finish = False statements in b8b9b4b.

[benoit74]

Sorry about that, but I think we need to fix the CHANGELOG as well, we've done way more than only handle "too many download failed" errors. You might keep existing entry and add a new one (mentioning something around cleaning up properly on exception during scraper run) pointing to this PR number (this is what we usually do when we have a change without a linked issue).

[benoit74]

LGTM, thank you