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 six-related code and __future__ imports #4121

Merged
merged 54 commits into from Dec 5, 2019
Merged

Remove six-related code and __future__ imports #4121

merged 54 commits into from Dec 5, 2019

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Nov 3, 2019

Based on #4115 and #4114, supersedes #4003

wRAR and others added 30 commits Oct 31, 2019
Co-Authored-By: Adrián Chaves <adrian@chaves.io>
@elacuesta elacuesta changed the title Remove six-related code Remove six-related code and __future__ imports Nov 3, 2019
@elacuesta elacuesta added this to the v2.0 milestone Nov 3, 2019
@elacuesta elacuesta mentioned this pull request Nov 3, 2019
@elacuesta elacuesta marked this pull request as ready for review Nov 3, 2019
Copy link
Member

@Gallaecio Gallaecio left a comment

Amazing work.

I’ve left a lot of feedback, but mostly aesthetic. Feel free to ignore and mark as resolve those aesthetic changes not affecting lines that you actually changed.

scrapy/exporters.py Show resolved Hide resolved
scrapy/extensions/feedexport.py Show resolved Hide resolved
scrapy/http/headers.py Show resolved Hide resolved
scrapy/linkextractors/sgml.py Show resolved Hide resolved
scrapy/settings/__init__.py Show resolved Hide resolved
tests/test_urlparse_monkeypatches.py Show resolved Hide resolved
tests/test_utils_datatypes.py Outdated Show resolved Hide resolved
tests/test_utils_deprecate.py Show resolved Hide resolved
tests/test_utils_misc/__init__.py Outdated Show resolved Hide resolved
tests/test_utils_python.py Outdated Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio left a comment

(accidental duplicate comment)

Co-Authored-By: Adrián Chaves <adrian@chaves.io>
@elacuesta
Copy link
Member Author

@elacuesta elacuesta commented Nov 5, 2019

@Gallaecio I agree with the proposed sorting suggestions, but I believe the PR is already large enough as it is (that said, I probably already sorted some of them). If you don't mind, I'd prefer to leave that import cleanup for separate PRs. What do you think?

@elacuesta
Copy link
Member Author

@elacuesta elacuesta commented Nov 22, 2019

Added two pickle-related checks to the bandit skip list (B301, B403). The problem appeared with the change from from six.moves import cPickle as pickle to import pickle. I'm not sure what else to do besides skipping the check, I thought #4135 might make a difference but I'm not sure, since B403 fails early at the import stage.

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Nov 22, 2019

Pickle is always a potential danger if it handles uncontrolled input data, I believe that’s why bandit checks it. If we are sure that a given pickle usage is safe, I think we can just mark that specific usage as such.

wRAR
wRAR approved these changes Dec 5, 2019
@wRAR wRAR merged commit 076f076 into master Dec 5, 2019
1 of 2 checks passed
@wRAR wRAR deleted the remove-six-code branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants