Skip to content

WIP Fix redirection bug.#261

Closed
rmax wants to merge 2 commits intoscrapinghub:masterfrom
rmax-contrib:scrapy-redirect-fix
Closed

WIP Fix redirection bug.#261
rmax wants to merge 2 commits intoscrapinghub:masterfrom
rmax-contrib:scrapy-redirect-fix

Conversation

@rmax
Copy link

@rmax rmax commented Feb 22, 2017

Scrapy's request meta holds native string as keys. This caused to a
redirected URL being added as seed again.

Scrapy's request meta holds native string as keys. This caused to a
redirected URL being added as seed again.
@sibiryakov
Copy link
Member

@rolando I think it's time to remove _request_is_redirected() method and it's call in enqueue_request(). Can you do that? If not, I could fix that.

@rmax
Copy link
Author

rmax commented Feb 22, 2017

@sibiryakov How does it look now? I don't know what was the rationale behind adding non-redirected URLs as seeds.

Let me know if it's OK and then I can squash the changes and remove the WIP status.

@codecov-io
Copy link

Codecov Report

Merging #261 into master will decrease coverage by -0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   70.15%   70.11%   -0.05%     
==========================================
  Files          68       68              
  Lines        4715     4715              
  Branches      632      576      -56     
==========================================
- Hits         3308     3306       -2     
- Misses       1267     1271       +4     
+ Partials      140      138       -2
Impacted Files Coverage Δ
frontera/contrib/scrapy/schedulers/frontier.py 96.69% <100%> (ø)
frontera/_version.py 30.14% <ø> (-1.48%)
frontera/contrib/messagebus/kafka/async.py 15.28% <ø> (ø)
frontera/utils/misc.py 68.18% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a442055...dcd2963. Read the comment docs.

return True
return False
self._add_pending_request(request)
self.stats_manager.add_redirected_requests()
Copy link
Member

Choose a reason for hiding this comment

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

that line is wrong, otherwise LGTM.

@sibiryakov
Copy link
Member

Could you fix the tests @rolando ?

@sibiryakov
Copy link
Member

Closing in favor #276

@sibiryakov sibiryakov closed this Apr 25, 2017
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.

3 participants