[MRG] Do not set Referer by default when its value is None #475
Conversation
|
I like more this approach than #474 |
|
Me too. How about a test? :) |
|
please review. |
| headers={'X-Set': '1', 'X-False': '', 'X-Unset': None}) | ||
| spider = SingleRequestSpider(seed=req) | ||
| yield docrawl(spider) | ||
| self.assertTrue('response' in spider.meta) |
There was a problem hiding this comment.
We can use self.assertIn because we dropped 2.6 support. I believe the advantage is a nicer failure message.
There was a problem hiding this comment.
addressed by dangra@3e84c70
|
I appreciate if someone else can provide some documentation wording for this feature before it's merged. |
|
Looks good to anyone? I'll merge if noone chime in :) |
|
Checked network traffic with Wireshark. |
|
I'd prefer an even more "functional-style" test - e.g. check that Referer header is not set when None is used and is set if None is not used (no explicit Referer header and explicit empty Referer header). Checking for setdefault looks like checking against implementation details. As for the docs, it seems that "unset" would be a bit misleading - we can't unset a header in Request constructor because the header can be added later by a middleware, and the feature should still work. It is more like "preventing header from being sent". |
|
@kmike: you're right, I'll add a test for Referer header and move the unittest to |
|
@kmike : review latest changes to tests cases. |
| # start requests doesn't set Referer header | ||
| echo0 = json.loads(spider.meta['responses'][2].body) | ||
| self.assertNotIn('Referer', echo0['headers']) | ||
| # following request set Referer to start request url |
|
Looks good! |
[MRG] Do not set Referer by default when its value is None
Another implementation for #473, alternative #474.
closes #473.