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

Pylint issues #5677

Merged
merged 15 commits into from
Nov 27, 2022
Merged

Pylint issues #5677

merged 15 commits into from
Nov 27, 2022

Conversation

marksmayo
Copy link
Contributor

Tidied up .pylintrc file, removed unnecessary ignores and fixed a bunch of simple ones.

Pylint issues - project had a lot ignored in the .pylintrc file, I systematically checked them all, fixed a bunch and removed some of the ignores.
@wRAR
Copy link
Member

wRAR commented Oct 12, 2022

Can you please look into the test failures and new flake8 messages?

.vscode/settings.json Outdated Show resolved Hide resolved
@wRAR
Copy link
Member

wRAR commented Oct 12, 2022

I think the test failure is because of the unidiomatic-typecheck "fix" (as the new code behaves differently in the case of a subclass).

@Gallaecio
Copy link
Member

@marksmayo Do you think you will find the time to work on addressing feedback here? Otherwise, @emarondan how would you feel about taking over and finishing this change yourself?

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #5677 (d384e09) into master (e2db624) will increase coverage by 0.18%.
The diff coverage is 91.08%.

@@            Coverage Diff             @@
##           master    #5677      +/-   ##
==========================================
+ Coverage   88.77%   88.95%   +0.18%     
==========================================
  Files         162      162              
  Lines       10958    10958              
  Branches     1793     1793              
==========================================
+ Hits         9728     9748      +20     
+ Misses        948      929      -19     
+ Partials      282      281       -1     
Impacted Files Coverage Δ
scrapy/commands/__init__.py 74.71% <0.00%> (ø)
scrapy/commands/parse.py 28.19% <ø> (ø)
scrapy/core/downloader/tls.py 97.05% <ø> (ø)
scrapy/core/downloader/webclient.py 94.65% <ø> (ø)
scrapy/core/http2/protocol.py 83.41% <0.00%> (ø)
scrapy/downloadermiddlewares/httpcompression.py 92.64% <ø> (ø)
scrapy/extensions/telnet.py 81.66% <ø> (ø)
scrapy/extensions/throttle.py 46.00% <ø> (ø)
scrapy/pipelines/files.py 71.33% <0.00%> (ø)
scrapy/robotstxt.py 97.53% <ø> (+22.22%) ⬆️
... and 38 more

@wRAR
Copy link
Member

wRAR commented Nov 25, 2022

flake8:

docs/conftest.py:15:1: E302 expected 2 blank lines, found 1
docs/utils/linkfix.py:34:9: F821 undefined name 'sys'
docs/_ext/scrapydocs.py:7:1: E302 expected 2 blank lines, found 1
scrapy/core/scraper.py:281:25: E127 continuation line over-indented for visual indent
scrapy/downloadermiddlewares/stats.py:6:1: E302 expected 2 blank lines, found 1
scrapy/downloadermiddlewares/redirect.py:60:21: E128 continuation line under-indented for visual indent
tests/test_downloadermiddleware_decompression.py:10:1: E303 too many blank lines (3)
tests/CrawlerProcess/reactor_default.py:7:1: E303 too many blank lines (3)
tests/CrawlerProcess/reactor_default_twisted_reactor_select.py:7:1: E303 too many blank lines (3)

@wRAR
Copy link
Member

wRAR commented Nov 25, 2022

Also, as we rolled back the changes, these message classes should be put back to pylintrc:

************* Module scrapy.http.request
scrapy/http/request/__init__.py:188:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
************* Module tests.test_request_cb_kwargs
tests/test_request_cb_kwargs.py:94:31: C1803: 'kwargs == {}' can be simplified to 'not kwargs' as an empty sequence is falsey (use-implicit-booleaness-not-comparison)

@marksmayo
Copy link
Contributor Author

Apologies, it's been a busy few weeks. Looking now

@marksmayo
Copy link
Contributor Author

@Gallaecio are you able to kick off the workflows, to see if any issues are left after I've resynched and fixed? Thanks.

@wRAR wRAR added this to the Scrapy 2.8 milestone Nov 26, 2022
@marksmayo
Copy link
Contributor Author

Thanks for your help! Was a bit of a big one to bite off for my first go, in hindsight

Comment on lines 3 to 6
from urllib.parse import urlparse, urlunparse, urldefrag

from twisted.web.http import HTTPClient

from twisted.internet import defer
Copy link
Member

Choose a reason for hiding this comment

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

Some import sorting and grouping changes like this one look wrong to me, but I am OK with merging this as an overall improvement, and in a separate pull request introduce a CI job to check these with isort, and include isort into the pre-commit configuration that we may introduce in #5734.

Comment on lines +32 to +33
host = netloc.split(":")[0]
self.logger.info(f"Host: {host}")
Copy link
Member

Choose a reason for hiding this comment

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

Just a tip for the future: using single quotes (':') would have worked as well to move netloc.split(":")[0] into the f-string.

@Gallaecio Gallaecio merged commit f9a29f0 into scrapy:master Nov 27, 2022
@Gallaecio
Copy link
Member

Thanks!

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.

4 participants