Attribute to control offsite filtering #3691
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3691 +/- ##
==========================================
+ Coverage 84.54% 84.71% +0.17%
==========================================
Files 167 168 +1
Lines 9420 9460 +40
Branches 1402 1407 +5
==========================================
+ Hits 7964 8014 +50
+ Misses 1199 1188 -11
- Partials 257 258 +1
|
|
LGTM |
scrapy/spidermiddlewares/offsite.py
Outdated
| @@ -28,7 +28,7 @@ def from_crawler(cls, crawler): | |||
| def process_spider_output(self, response, result, spider): | |||
| for x in result: | |||
| if isinstance(x, Request): | |||
| if x.dont_filter or self.should_follow(x, spider): | |||
| if x.meta.get('allow_offsite_requests', False) or self.should_follow(x, spider): | |||
There was a problem hiding this comment.
I am thinking this would keep this change backward-compatible:
| if x.meta.get('allow_offsite_requests', False) or self.should_follow(x, spider): | |
| if x.meta.get('allow_offsite_requests', x.dont_filter) or self.should_follow(x, spider): |
If you agree, then I suggest you also change your test changes, so that you keep existing tests unchanged and simply add additional tests scenarios.
There was a problem hiding this comment.
I'm not too sure about this even with the advantage of backward compatibility because when we want to duplicate a request but not skip the offsite filter ( which I assume is the case for majority of instances where we set dont_filter=True) we have to add meta={'allow_offsite': False} which is extra work. Plus there will be two ways to skip offsite filtering.
Hence I'm proposing to do this in a backward incompatible way if possible.
Gallaecio
dismissed
their stale review
Author might have a point about making this backward-incompatible on purpose being the best choice, but I am not sure.
This is a fix for #3690