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

[MRG+1] change the bad smell code #3304

Merged
merged 4 commits into from Jul 3, 2018
Merged

[MRG+1] change the bad smell code #3304

merged 4 commits into from Jul 3, 2018

Conversation

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jun 23, 2018

  • re-order the import, to keep it clear
  • fix the bad smell code, don't use a reserved keyword as a variable name
@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Jun 23, 2018

Tests failed, I need to check what's wrong.

@grammy-jiang grammy-jiang deleted the scrapedia:dev branch Jun 23, 2018
@grammy-jiang grammy-jiang restored the scrapedia:dev branch Jun 28, 2018
@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Jun 28, 2018

Since the build fixed, reopen this PR again.

@grammy-jiang grammy-jiang reopened this Jun 28, 2018
@codecov
Copy link

@codecov codecov bot commented Jun 28, 2018

Codecov Report

Merging #3304 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3304   +/-   ##
=======================================
  Coverage   82.14%   82.14%           
=======================================
  Files         228      228           
  Lines        9605     9605           
  Branches     1387     1387           
=======================================
  Hits         7890     7890           
  Misses       1458     1458           
  Partials      257      257
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/httpproxy.py 100% <100%> (ø) ⬆️
@@ -1,14 +1,14 @@
import base64
from six.moves.urllib.request import getproxies, proxy_bypass
from six.moves.urllib.parse import unquote

This comment has been minimized.

@kmike

kmike Jun 28, 2018
Member

We usually put six.moves imports in the same group as stdlib import, because in a way they are stdlib imports, just with locations compatible with Python 2 and Python 3. When Scrapy supported only Python 2, these imports were in a group of stdlib imports, when Scrapy will support only Python 3, these imports will again be just stdlib imports. This should be rather consistent in other Scrapy files as well; I'd prefer not to change it.

This comment has been minimized.

@grammy-jiang

grammy-jiang Jun 28, 2018
Author Contributor

I agree. I will just put the two imports from six.moves.urllib.parse together.

for type, url in getproxies().items():
self.proxies[type] = self._get_proxy(url, type)
for type_, url in getproxies().items():
self.proxies[type_] = self._get_proxy(url, type_)

This comment has been minimized.

@kmike

kmike Jun 28, 2018
Member

👍

return cls(auth_encoding)
obj = cls(auth_encoding)

return obj

This comment has been minimized.

@kmike

kmike Jun 28, 2018
Member

I'm not sure changes to this method add to clarity. pep8 says: "Use blank lines in functions, sparingly, to indicate logical sections." (emphasis is mine).

This comment has been minimized.

@grammy-jiang

grammy-jiang Jun 28, 2018
Author Contributor

Agreed, I will revert this change.

@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Jun 28, 2018

hi, @kmike,

Thanks for your comments. I have made new changes based on your comments, please review.

@kmike kmike changed the title change the bad smell code [MRG+1] change the bad smell code Jun 29, 2018
@kmike
Copy link
Member

@kmike kmike commented Jun 29, 2018

Looks good to me, thanks @grammy-jiang! +1 to squash and merge.

@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Jun 29, 2018

Thanks, @kmike !

Nowadays I am working on this issue #3195 , and I will need your help a lot!

@dangra dangra merged commit d05c867 into scrapy:master Jul 3, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grammy-jiang grammy-jiang deleted the scrapedia:dev branch Jul 4, 2018
@kmike kmike added this to the v1.6 milestone Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants