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

5245 f-strings #5246

Merged
merged 32 commits into from Oct 7, 2021
Merged

5245 f-strings #5246

merged 32 commits into from Oct 7, 2021

Conversation

GeorgeA92
Copy link
Contributor

@GeorgeA92 GeorgeA92 commented Sep 23, 2021

Related to #5245
Replaced string format by usage if f-strings in places mentioned in recent pylint checks.

(edit) fixes #5245

@GeorgeA92 GeorgeA92 changed the title 5242 f-strings 5245 f-strings Sep 23, 2021
logger.debug("Using DBM cache storage in %(cachepath)s" % {'cachepath': dbpath}, extra={'spider': spider})
logger.debug(f"Using DBM cache storage in {dbpath}", extra={'spider': spider})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old message from pylint related to this code line:
scrapy/extensions/httpcache.py:229:21: C0209: Formatting a regular string which could be a f-string (consider-using-f-string)
New message from pylint related to this code line:
scrapy/extensions/httpcache.py:229:8: W1203: Use lazy % or .format() or % formatting in logging functions (logging-fstring-interpolation)

@@ -70,7 +70,7 @@ def process_request(self, request: Request, spider: Spider) -> Optional[Response
self.stats.inc_value('httpcache/miss', spider=spider)
if self.ignore_missing:
self.stats.inc_value('httpcache/ignore', spider=spider)
raise IgnoreRequest("Ignored request not in cache: %s" % request)
raise IgnoreRequest(f"Ignored request not in cache: {str(request)}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise IgnoreRequest(f"Ignored request not in cache: {str(request)}")
raise IgnoreRequest(f"Ignored request not in cache: {request}")

Use str inside a f-string is redudant, same for others places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks @Laerte

Copy link
Member

Choose a reason for hiding this comment

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

@GeorgeA92 No problem, great work btw.

Copy link
Member

Choose a reason for hiding this comment

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

I think this wasn't implemented?

@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #5246 (a4cc04c) into master (8284de5) will increase coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #5246      +/-   ##
==========================================
+ Coverage   88.51%   88.53%   +0.02%     
==========================================
  Files         163      163              
  Lines       10592    10611      +19     
  Branches     1524     1559      +35     
==========================================
+ Hits         9375     9394      +19     
+ Misses        943      942       -1     
- Partials      274      275       +1     
Impacted Files Coverage Δ
scrapy/__init__.py 84.21% <0.00%> (ø)
scrapy/core/downloader/contextfactory.py 87.03% <0.00%> (ø)
scrapy/core/downloader/tls.py 97.14% <ø> (ø)
scrapy/utils/misc.py 97.72% <ø> (ø)
scrapy/extensions/feedexport.py 95.08% <75.00%> (-0.02%) ⬇️
scrapy/downloadermiddlewares/httpcache.py 93.97% <100.00%> (ø)
scrapy/extensions/httpcache.py 95.43% <100.00%> (ø)
scrapy/commands/check.py 71.01% <0.00%> (ø)
scrapy/downloadermiddlewares/httpauth.py 100.00% <0.00%> (ø)

@wRAR
Copy link
Member

wRAR commented Sep 24, 2021

Looks like there is now a TypeError in the spider ran in tests/test_crawl.py::CrawlSpiderTestCase::test_async_def_asyncio_parse_reqs_list

@GeorgeA92
Copy link
Contributor Author

@wRAR remaining errors fixed. checks/tests passed

scrapy/core/downloader/contextfactory.py Outdated Show resolved Hide resolved
scrapy/core/downloader/tls.py Outdated Show resolved Hide resolved
scrapy/extensions/feedexport.py Outdated Show resolved Hide resolved
scrapy/utils/misc.py Outdated Show resolved Hide resolved
tests/test_http_request.py Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

I believe the only thing missing now is #5246 (comment)

tests/test_http_request.py Outdated Show resolved Hide resolved
Comment on lines +378 to +379
"Stored %s", logmsg,
extra={'spider': spider}
Copy link
Member

Choose a reason for hiding this comment

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

(Nitpick) I would suggest either putting everything in the same line (which should be ok given our current line length), or each argument in its own line.

@GeorgeA92
Copy link
Contributor Author

GeorgeA92 commented Oct 1, 2021

@Gallaecio
On last check received failed tests (pypy) on CallbackKeywordArgumentsTestCase.test_callback_kwargs.
Probably related to recently (14 hours ago) megred #5251

@Gallaecio
Copy link
Member

Gallaecio commented Oct 2, 2021

Yeap, @elacuesta was aware and left the work to you. Bad @elacuesta 🙂

@Gallaecio
Copy link
Member

@GeorgeA92 Also, please, do not forget to address #5246 (comment) , I think you forgot about that at some point.

@Gallaecio
Copy link
Member

Gallaecio commented Oct 2, 2021

Yeap, @elacuesta was aware and left the work to you. Bad @elacuesta 🙂

Actually, I think I misinterpreted his comment.

@Gallaecio Gallaecio closed this Oct 7, 2021
@Gallaecio Gallaecio reopened this Oct 7, 2021
@Gallaecio Gallaecio merged commit d3f1bf7 into scrapy:master Oct 7, 2021
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.

Complete conversion to f-strings
5 participants