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

[MRG+1] add item_error to be catchable #3256

Merged
merged 1 commit into from Jul 3, 2018
Merged

[MRG+1] add item_error to be catchable #3256

merged 1 commit into from Jul 3, 2018

Conversation

@chainly
Copy link
Contributor

@chainly chainly commented May 11, 2018

add item_error to be catchable (ie. by extension with signal).

@lucywang000
Copy link
Member

@lucywang000 lucywang000 commented May 16, 2018

@chainly can you explain the motivation for this, like a typical use case to justify this new signal? What's more I think a new test case is required.

@codecov
Copy link

@codecov codecov bot commented May 16, 2018

Codecov Report

Merging #3256 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3256      +/-   ##
==========================================
+ Coverage   82.12%   82.15%   +0.03%     
==========================================
  Files         228      228              
  Lines        9599     9601       +2     
  Branches     1385     1385              
==========================================
+ Hits         7883     7888       +5     
+ Misses       1457     1454       -3     
  Partials      259      259
Impacted Files Coverage Δ
scrapy/signals.py 100% <100%> (ø) ⬆️
scrapy/core/scraper.py 88.51% <100%> (+2.11%) ⬆️
@chainly
Copy link
Contributor Author

@chainly chainly commented May 16, 2018

I want to count/know how many item dropped by unhandled exception occurred in pipeline easily , but only printed in log.

@kmike kmike changed the title add item_error to be catchable [MRG+1 add item_error to be catchable May 16, 2018
@kmike kmike changed the title [MRG+1 add item_error to be catchable [MRG+1] add item_error to be catchable May 16, 2018
@kmike
Copy link
Member

@kmike kmike commented May 16, 2018

This feature makes sense to me, and implementation looks good. The only question I have is why is Failure returned, not an exception, like in item_dropped?

@chainly
Copy link
Contributor Author

@chainly chainly commented May 17, 2018

  1. There are mistake in test case(self.run.itemerror ==> self.run.itemresp), now fix. Thanks to the question : )
  2. For the question: if someone need traceback, Failure a good choise, see the doc failure.Failure, and failure.Failure.getTracebackObject. Why lost, not known.
# test code
def item_error(self, item, response, spider, failure):
    import traceback
    print('exception')
    traceback.print_exc()
    self.itemerror.append((item, response, spider, failure))

def _assert_items_error(self):
    self.assertEqual(2, len(self.run.itemerror))
    for item, response, spider, failure in self.run.itemerror:
        self.assertEqual(failure.value.__class__, ZeroDivisionError)
        import traceback
        print('failure')
        traceback.print_tb(failure.getTracebackObject())
        self.assertEqual(spider,self.run.spider2)

# output
--------------------------------------------------------------------------- Captured stdout call ----------------------------------------------------------------------------
exception
exception
failure
--------------------------------------------------------------------------- Captured stderr call ----------------------------------------------------------------------------
NoneType: None
NoneType: None
  File "/Users/xxx/anaconda/envs/python36/lib/python3.6/site-packages/twisted/internet/defer.py", line 653, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/test/ttttt/scrapy/tests/pipelines.py", line 17, in process_item
    1/0
@whalebot-helmsman
Copy link
Contributor

@whalebot-helmsman whalebot-helmsman commented May 17, 2018

@chainly Problem with unit-tests was fixed in master. Can you merge master to this PR?

@chainly
Copy link
Contributor Author

@chainly chainly commented May 18, 2018

OK, fast-forwarded and push again.

@dangra dangra merged commit 74ce156 into scrapy:master Jul 3, 2018
2 checks passed
2 checks passed
codecov/patch 100% of diff hit (target 82.12%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike kmike added this to the v1.6 milestone Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants