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

Finish exporting for each start exporting #5758

Merged

Conversation

MattyMay
Copy link
Contributor

@MattyMay MattyMay commented Dec 12, 2022

Figured I'd take a crack at #5537 since it's been stale for a while.

This patch moves the call to slot.finish_exporting() up to before the conditional that checks if slot.item_count > 0 so that if an exception occurs when an exporter's export_item() method is called, slot.finish_exporting() will still be called (as discussed in the issue).

I've included 4 new tests to that each check that:

  1. if slot.start_exporting() was called that there is a call to slot.finish_exporting() after
  2. slot.finish_exporting() is not called before a call to slot.start_exporting(). I.e. they assert that slot.finish_exporting() isn't called twice in a row without a call to slot.start_exporting() in between and that slot.finish_exporting() isn't called before an initial call to slot.start_exporting.

test_start_finish_exporting_items_exception fails before the patch and passes after the patch. The other 3 tests pass before and after.

Fixes #5537

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #5758 (1ab9006) into master (034dc8f) will increase coverage by 4.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5758      +/-   ##
==========================================
+ Coverage   84.82%   88.87%   +4.04%     
==========================================
  Files         162      162              
  Lines       10974    10980       +6     
  Branches     1795     1796       +1     
==========================================
+ Hits         9309     9758     +449     
+ Misses       1394      941     -453     
- Partials      271      281      +10     
Impacted Files Coverage Δ
scrapy/extensions/feedexport.py 95.45% <100.00%> (+4.22%) ⬆️
scrapy/shell.py 68.65% <0.00%> (-1.05%) ⬇️
scrapy/utils/defer.py 97.31% <0.00%> (-0.02%) ⬇️
scrapy/utils/reactor.py 84.09% <0.00%> (+0.95%) ⬆️
scrapy/squeues.py 100.00% <0.00%> (+4.54%) ⬆️
scrapy/pipelines/media.py 98.58% <0.00%> (+5.67%) ⬆️
scrapy/utils/test.py 66.21% <0.00%> (+9.45%) ⬆️
scrapy/core/downloader/contextfactory.py 87.03% <0.00%> (+11.11%) ⬆️
scrapy/robotstxt.py 88.88% <0.00%> (+13.58%) ⬆️
... and 5 more

@Gallaecio
Copy link
Member

Closing and reopening to trigger new CI jobs…

@Gallaecio Gallaecio closed this Jan 11, 2023
@Gallaecio Gallaecio reopened this Jan 11, 2023
tests/test_feedexport.py Outdated Show resolved Hide resolved
@wRAR wRAR merged commit 604ddf8 into scrapy:master Jan 12, 2023
@wRAR
Copy link
Member

wRAR commented Jan 12, 2023

Thanks!

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.

Might be nice to always call exporter.finish_exporting
3 participants