Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Callback kwargs #3563
[MRG+1] Callback kwargs #3563
Changes from 7 commits
50a0d87
a2b509a
69a1ee7
a67f1ce
770a501
57e7c76
bddfeab
645e8d1
6760bca
8528f50
c43a231
70a4d93
3efe3be
e8af633
8fb0776
f5e0b6b
ccb56a3
294ef51
0522fe3
428309b
1f9f41b
d4d68cf
312e573
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test which checks how Scrapy behaves if there is a mismatch between parameters
parse
accept and parameters passed via callback kwargs, e.g. some required argument is missing, or an extra argument is passed? Is exception raised? Does traceback look good (no need to write a test for it)? Is errback called?Another case which could be nice to check explicitly is how default values are handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for defaults and argument mismatch.
Errback is not called with this current implementation, it does if we add the callback and the errback to the Deferred in two separate steps, i.e.
instead of
In any case, the
Request
object is not bounded to theFailure
received by the errback. Personally, I don't think calling the errback would be appropriate here, since it's not an error with the request/response itself, but with the code that handles it. The logged error is very descriptive, and similar to the one that currently appears when, for instance, a callback does not take a second positional argument (TypeError: parse() takes 1 positional argument but 2 were given
).