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

refactor: use f-strings #4324

Merged
merged 12 commits into from Aug 29, 2020
Merged

refactor: use f-strings #4324

merged 12 commits into from Aug 29, 2020

Conversation

ammarnajjar
Copy link
Contributor

@ammarnajjar ammarnajjar commented Feb 9, 2020

Resolves #4307, closes #4672

@ammarnajjar ammarnajjar mentioned this pull request Feb 9, 2020
@Gallaecio Gallaecio added this to the Python 3.6+ milestone Feb 10, 2020
@simulacra342870
Copy link

simulacra342870 commented Feb 15, 2020

I think I have the solution
I have just refactored this function within the scrapy/core/spidermw.py file:

def _fname(f):
return "%s.%s".format(
f.self.class.name,
f.func.name
)

to fstrings with same placeholders, eg %s for strings,

def _fname(f):
return f"%s.%s"(
f.self.class.name,
f.func.name
)

Don't hurt me I am extremely newbie, I am only trying to help and gain experience.
Are there any other files within the scrapy source code that needs to be refactored or do Ihave to look through all of source code to find a way to use fstrings to refactor the code.

cheers danny

@ammarnajjar ammarnajjar changed the title refactor: use f-strings WIP: refactor: use f-strings Feb 15, 2020
@Gallaecio
Copy link
Member

Gallaecio commented Feb 17, 2020

Are there any other files within the scrapy source code that needs to be refactored or do Ihave to look through all of source code to find a way to use fstrings to refactor the code.

Since @ammarnajjar is already working on this, I think it would be better if you, @simulacra342870, worked on a different kind of refactoring.

We recently added Flake8 checks to the Scrapy code base, but we have not finished fixing all of them yet. If you look under “Issues pending a review” in https://github.com/scrapy/scrapy/blob/master/pytest.ini you will see pending Flake8 issues that we want to fix. You could choose one of those issue codes (except for E501), remove the code from the corresponding lines from pytest.ini (removing the whole line where that was the only code), run tox -e flake8 locally to find the issues (see https://docs.scrapy.org/en/master/contributing.html#tests), and fix all reported issues.

@Gallaecio Gallaecio mentioned this pull request Feb 17, 2020
@simulacra342870
Copy link

simulacra342870 commented Feb 17, 2020

Thanks I don't want to step on anybody's toes, i'm just a newbie looking for experience and a project to build up portfolio, etc.

Also another point of order, did my fix seem correct for you, was the coding correct, as this is very important to me.

Thanks for taking time to reply and suggest another project, I'm always looking for new projects, so if you have any future biggish newbie projects, tat would even take quite a bit of time, I would be up for this, just contact me through Github.

@kmike
Copy link
Member

kmike commented Feb 19, 2020

hey @ammarnajjar! The changes look good on a first sight (haven't checked them in detail), but I'm pessimistic about getting this PR merged; we'll be getting more and more merge conflicts. I think it makes sense to get back to it after Python 3.5 support is dropped.

@ammarnajjar
Copy link
Contributor Author

ammarnajjar commented Feb 20, 2020

@kmike, we're on the same page => #4307 (comment)

@simulacra342870
Copy link

simulacra342870 commented Feb 20, 2020

Is kmike talking about my code /fix above or something ammarnajjar did I'm confused, I just wanted to know if what I did is correct then go on to the next problem that ammarnajjar suggested.
Cheers

@kmike
Copy link
Member

kmike commented Feb 20, 2020

@simulacra342870 I'm talking about @ammarnajjar's proposed changes. It seems it is not a right time to work on f-strings now.

@elacuesta elacuesta mentioned this pull request Jul 10, 2020
@Gallaecio Gallaecio modified the milestones: Python 3.6+, 2.4 Aug 17, 2020
@Gallaecio
Copy link
Member

Gallaecio commented Aug 17, 2020

@ammarnajjar The next Scrapy version is expected to drop Python 3.5 support 🎉. Do you think you will be able to update your pull request in the following few weeks?

@ammarnajjar
Copy link
Contributor Author

ammarnajjar commented Aug 17, 2020

@Gallaecio sure!

@ammarnajjar ammarnajjar force-pushed the 4307-use-f-strings branch 3 times, most recently from 0f107a1 to e5e7952 Compare Aug 23, 2020
@codecov
Copy link

codecov bot commented Aug 23, 2020

Codecov Report

Merging #4324 into master will increase coverage by 0.01%.
The diff coverage is 69.56%.

@@            Coverage Diff             @@
##           master    #4324      +/-   ##
==========================================
+ Coverage   87.19%   87.21%   +0.01%     
==========================================
  Files         160      160              
  Lines        9812     9813       +1     
  Branches     1447     1447              
==========================================
+ Hits         8556     8558       +2     
+ Misses        995      994       -1     
  Partials      261      261              
Impacted Files Coverage Δ
scrapy/commands/__init__.py 67.14% <ø> (ø)
scrapy/commands/edit.py 44.44% <0.00%> (ø)
scrapy/commands/parse.py 19.77% <0.00%> (ø)
scrapy/core/downloader/handlers/__init__.py 83.63% <0.00%> (ø)
scrapy/core/downloader/middleware.py 96.36% <ø> (ø)
scrapy/core/downloader/webclient.py 94.48% <0.00%> (ø)
scrapy/core/engine.py 87.50% <0.00%> (ø)
scrapy/core/scraper.py 87.11% <0.00%> (ø)
scrapy/downloadermiddlewares/httpproxy.py 100.00% <ø> (ø)
scrapy/extensions/debug.py 46.15% <0.00%> (ø)
... and 70 more

@ammarnajjar
Copy link
Contributor Author

ammarnajjar commented Aug 23, 2020

@Gallaecio, I have done the changes, but these lines decrease the coverage:

if sys.version_info < (3, 5, 2):
print("Scrapy %s requires Python 3.5.2" % __version__)
sys.exit(1)

@ammarnajjar ammarnajjar changed the title WIP: refactor: use f-strings refactor: use f-strings Aug 24, 2020
@Gallaecio
Copy link
Member

Gallaecio commented Aug 25, 2020

Actually, I don’t think we should use f-strings there, as that is meant to be executed in earlier Python versions.

Copy link
Member

@Gallaecio Gallaecio left a comment

Impressive work!

It took so long to review that I’m glad I have a few inline comments to show for it 😅

PS: For further changes, please do not force-push, as it makes it hard to tell what changed since a previous review. Don’t worry about the commit history, we can squash before merging.

scrapy/commands/version.py Outdated Show resolved Hide resolved
scrapy/core/downloader/__init__.py Outdated Show resolved Hide resolved
scrapy/extensions/statsmailer.py Outdated Show resolved Hide resolved
scrapy/http/request/form.py Outdated Show resolved Hide resolved
scrapy/http/response/__init__.py Show resolved Hide resolved
scrapy/utils/conf.py Outdated Show resolved Hide resolved
scrapy/utils/conf.py Outdated Show resolved Hide resolved
sep/sep-002.rst Outdated Show resolved Hide resolved
ammarnajjar and others added 2 commits Aug 26, 2020
Co-authored-by: Adrián Chaves <adrian@chaves.io>
Co-authored-by: Adrián Chaves <adrian@chaves.io>
@kmike
Copy link
Member

kmike commented Aug 28, 2020

Hey! Sorry, I introduced a conflict while merging #4764 - could you please check it?

@ammarnajjar
Copy link
Contributor Author

ammarnajjar commented Aug 29, 2020

@kmike , no problem, done.

scrapy/commands/check.py Outdated Show resolved Hide resolved
Co-authored-by: Mikhail Korobov <kmike84@gmail.com>
@kmike
Copy link
Member

kmike commented Aug 29, 2020

Thanks for a careful cleanup, and for your responsiveness @ammarnajjar!

@kmike kmike merged commit 5e9cc32 into scrapy:master Aug 29, 2020
1 of 2 checks passed
@ammarnajjar ammarnajjar deleted the 4307-use-f-strings branch Aug 29, 2020
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.

Use f-strings
4 participants