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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CallbackKeywordArgumentsContract #3988

Merged
merged 5 commits into from Sep 7, 2019
Merged

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Aug 29, 2019

Fixes #3985

This patch works partially, in the sense that it prevents the reported TypeError (I checked, for instance by adding a simple print(request.cb_kwargs) to https://github.com/scrapy/scrapy/blob/1.7.3/scrapy/core/scheduler.py#L90, and the generated request does include a valid cb_kwargs attribute). However, adding the @cb_kwargs contract causes the whole callback to be excluded from the tests, and I can't for the life of me figure out why.

Update: Working now, thanks to @victor-torres 馃殌

Remaining tasks:

  • Docs
  • Tests
  • Figure out why it doesn't work

@codecov
Copy link

codecov bot commented Aug 29, 2019

Codecov Report

Merging #3988 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3988      +/-   ##
==========================================
- Coverage   85.39%   85.38%   -0.02%     
==========================================
  Files         167      167              
  Lines        9724     9730       +6     
  Branches     1456     1456              
==========================================
+ Hits         8304     8308       +4     
- Misses       1162     1164       +2     
  Partials      258      258
Impacted Files Coverage 螖
scrapy/settings/default_settings.py 98.7% <酶> (酶) 猬嗭笍
scrapy/contracts/__init__.py 81.3% <66.66%> (酶) 猬嗭笍
scrapy/contracts/default.py 84% <66.66%> (-2.37%) 猬囷笍

@@ -92,7 +92,7 @@ def _clean_req(self, request, method, results):
@wraps(cb)
def cb_wrapper(response):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're wrapping the callback, you need to receive the kwargs here as well.

@wraps(cb)
def cb_wrapper(response, **cb_kwargs):
    pass

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to do it in the other wrappers.

@@ -92,7 +92,7 @@ def _clean_req(self, request, method, results):
@wraps(cb)
def cb_wrapper(response):
try:
output = cb(response)
output = cb(response, **request.cb_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, instead of using the synthetic request's kwargs, you need to pass the wrapped kwargs to the callback.

try:
    output = cb(response, **cb_kwargs)
except Exception:
    ...

@victor-torres
Copy link
Contributor

Despite of that, you might have some problems when trying to pass a Python object through the cb_kwargs because it won't be possible to serialize and deserialize data.

@elacuesta
Copy link
Member Author

That's true, but it's a current limitation of Request.meta as well, and it's solved by enqueuing to memory instead of disk. I don't think we need to worry about that in the context of Scrapy contracts though, since the requests are built from docstings and the cb_kwargs attribute in particular is decoded as JSON.

@elacuesta elacuesta marked this pull request as ready for review August 31, 2019 05:44
@kmike kmike merged commit 534de73 into scrapy:master Sep 7, 2019
@kmike
Copy link
Member

kmike commented Sep 7, 2019

Thanks @elacuesta, and thanks @victor-torres and @Gallaecio for reviews!

@elacuesta elacuesta deleted the contracts_cb_kwargs branch September 8, 2019 18:01
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.

Contracts can't be written for callbacks that receive cb_kwargs
4 participants