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

Remove py2 code #4115

Merged
merged 13 commits into from Nov 14, 2019
Merged

Remove py2 code #4115

merged 13 commits into from Nov 14, 2019

Conversation

wRAR
Copy link
Contributor

@wRAR wRAR commented Oct 31, 2019

This removes most of Py2-only code, except six wrappers and stuff covered in #4003 and #4114.

This also deprecates scrapy.utils.gz.read1 and scrapy.utils.python.to_native_str.

@wRAR wRAR added the Python 3 label Oct 31, 2019
@wRAR wRAR added this to the v2.0 milestone Oct 31, 2019
Copy link
Member

@elacuesta elacuesta left a comment

Looking good! Do you already have code to remove six.iteritems, six.text_type, etc? I can help you with that if you didn't start yet.

@@ -4,18 +4,14 @@

import bz2
import gzip
from io import BytesIO
Copy link
Member

@elacuesta elacuesta Oct 31, 2019

Choose a reason for hiding this comment

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

Nitpick: could you group the ^import ... and ^from ... lines together? There are a couple other occurrences of this in PR.

Copy link
Contributor Author

@wRAR wRAR Oct 31, 2019

Choose a reason for hiding this comment

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

All ^import together and then all ^from together? I've noticed only recently that @Gallaecio is promoting this style, I didn't use it before.

Copy link
Member

@Gallaecio Gallaecio Nov 1, 2019

Choose a reason for hiding this comment

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

I must say I didn’t use it either until recently. I think I found about it in some pull request, and https://stackoverflow.com/a/20763446/939364 seems to confirm that this is the convention.

@wRAR
Copy link
Contributor Author

wRAR commented Oct 31, 2019

@elacuesta no, I didn't start working on other six stuff.

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #4115 into master will increase coverage by 0.29%.
The diff coverage is 92.53%.

@@            Coverage Diff             @@
##           master    #4115      +/-   ##
==========================================
+ Coverage   83.38%   83.68%   +0.29%     
==========================================
  Files         165      165              
  Lines        9801     9713      -88     
  Branches     1463     1445      -18     
==========================================
- Hits         8173     8128      -45     
+ Misses       1364     1336      -28     
+ Partials      264      249      -15
Impacted Files Coverage Δ
scrapy/http/response/__init__.py 93.65% <ø> (ø) ⬆️
scrapy/core/downloader/handlers/http11.py 91.73% <ø> (ø) ⬆️
scrapy/crawler.py 89.94% <ø> (+2%) ⬆️
scrapy/downloadermiddlewares/robotstxt.py 100% <ø> (ø) ⬆️
scrapy/_monkeypatches.py 100% <ø> (+45.45%) ⬆️
scrapy/extensions/feedexport.py 81.81% <0%> (-0.09%) ⬇️
scrapy/utils/boto.py 75% <0%> (+28.33%) ⬆️
scrapy/pipelines/images.py 90.38% <100%> (-0.27%) ⬇️
scrapy/downloadermiddlewares/httpproxy.py 100% <100%> (ø) ⬆️
scrapy/utils/request.py 100% <100%> (ø) ⬆️
... and 27 more

Copy link
Member

@Gallaecio Gallaecio left a comment

Regarding to_unicode, I feel like we can get rid of many of its usages, if not all. But I guess we can do that in a separate changeset.

docs/topics/downloader-middleware.rst Outdated Show resolved Hide resolved
@@ -4,18 +4,14 @@

import bz2
import gzip
from io import BytesIO
Copy link
Member

@Gallaecio Gallaecio Nov 1, 2019

Choose a reason for hiding this comment

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

I must say I didn’t use it either until recently. I think I found about it in some pull request, and https://stackoverflow.com/a/20763446/939364 seems to confirm that this is the convention.

@elacuesta
Copy link
Member

elacuesta commented Nov 3, 2019

@elacuesta no, I didn't start working on other six stuff.

Great, I opened #4121, I'd appreciate your feedback there.

tests/test_link.py Outdated Show resolved Hide resolved
tests/test_utils_python.py Outdated Show resolved Hide resolved
@kmike
Copy link
Member

kmike commented Nov 14, 2019

Thanks @wRAR, and thanks @elacuesta and @Gallaecio for the feedback! Let's merge it.

@kmike kmike merged commit 494f38a into master Nov 14, 2019
2 checks passed
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.

None yet

4 participants