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] Update _monkeypatches.py #3907
Conversation
The workarounds are not required assuming the bugs regarding urlparse are absent in Python versions >2.7. We already exit the program if Python version<2.7 in the __init__.py(line 17).The monkeypatches are deployed after this check at line 27 in the __init__.py .
if six.PY2: | ||
from urlparse import urlparse | ||
|
||
# workaround for https://bugs.python.org/issue7904 - Python < 2.7 |
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.
👍
scrapy/_monkeypatches.py
Outdated
from urlparse import uses_netloc | ||
uses_netloc.append('s3') | ||
|
||
# workaround for https://bugs.python.org/issue9374 - Python < 2.7.4 |
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.
I think we shouldn't remove the second workaround, as it may be needed in 2.7 < Python < 2.7.4
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.
Yep my bad, python 2.7.4 is still permitted . I am newbie to the whole project so this may sound stupid but why is the Travis CI build is failing? I noticed it with other pull requests too.
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 the second workaround in the new commit.Could you review the new code?
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 the second workaround.
Codecov Report
@@ Coverage Diff @@
## master #3907 +/- ##
==========================================
- Coverage 85.47% 84.92% -0.55%
==========================================
Files 165 165
Lines 9624 9615 -9
Branches 1446 1443 -3
==========================================
- Hits 8226 8166 -60
- Misses 1145 1189 +44
- Partials 253 260 +7
|
Codecov Report
@@ Coverage Diff @@
## master #3907 +/- ##
==========================================
+ Coverage 85.47% 85.48% +<.01%
==========================================
Files 165 165
Lines 9624 9622 -2
Branches 1446 1446
==========================================
- Hits 8226 8225 -1
+ Misses 1145 1144 -1
Partials 253 253
|
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.
Thanks for your patch, but looking at the changes I see a few unrelated format changes, some of them for the worse. Could you please make your changes only remove the unnecessary workaround?
Sorry for the formatting. Anyway I have updated the code accordingly.Please review. |
scrapy/_monkeypatches.py
Outdated
# workaround for https://bugs.python.org/issue9374 - Python < 2.7.4 | ||
if urlparse('s3://bucket/key?key=value').query != 'key=value': | ||
from urlparse import uses_query | ||
uses_query.append('s3') | ||
|
||
|
||
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.
You missed a spot 🙂
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.
Fixed I hope.
Thanks @sbs2001, and thanks @Gallaecio for the review! |
The workarounds are not required assuming the bugs regarding urlparse are absent in Python versions >2.7. We already exit the program if Python version<2.7 in the init.py(line 17).The monkeypatches are deployed after this check at line 27 in the init.py .