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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Fix redirection bug. #261

Closed
wants to merge 2 commits into from

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