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

applying black to all the code #5734

Merged
merged 17 commits into from
Jan 25, 2023
Merged

Conversation

emarondan
Copy link
Contributor

@emarondan emarondan commented Nov 24, 2022

Applying black formatter to all scrapy code as part of an internal task.

Closes #4658 and closes #4654.

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #5734 (72a853c) into master (9cb757d) will not change coverage.
The diff coverage is 87.58%.

@@           Coverage Diff           @@
##           master    #5734   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files         162      162           
  Lines       10988    10988           
  Branches     1797     1797           
=======================================
  Hits         9772     9772           
  Misses        937      937           
  Partials      279      279           
Impacted Files Coverage Δ
scrapy/__main__.py 0.00% <0.00%> (ø)
scrapy/commands/crawl.py 60.00% <0.00%> (ø)
scrapy/extensions/debug.py 46.15% <0.00%> (ø)
scrapy/extensions/memdebug.py 47.61% <0.00%> (ø)
scrapy/extensions/statsmailer.py 0.00% <ø> (ø)
scrapy/http/common.py 100.00% <ø> (ø)
scrapy/interfaces.py 100.00% <ø> (ø)
scrapy/loader/common.py 100.00% <ø> (ø)
scrapy/utils/asyncgen.py 100.00% <ø> (ø)
scrapy/utils/benchserver.py 28.12% <0.00%> (ø)
... and 141 more

@wRAR
Copy link
Member

wRAR commented Nov 24, 2022

Thanks!

Several points I want to mention:

  1. If you ran black with non-default options (e.g. with a non-default line length) you should add a config so that simply running black has the same outcome.
  2. Can you please mention in the original message that this PR closes Use black python formatter #4658 and Blank lines after import statements #4654 so that they are autoclosed?
  3. I see that both pylint and flake8 checks failed, we may need to silence the related warnings if they conflict with the black standard.
  4. As mentioned in Use black python formatter #4658, we need to add .git-blame-ignore-revs, but probably as a separate PR?
  5. And finally, I feel like we should merge this only after both recent huge PRs (Pylint issues #5677 and use pathlib #5682) are merged, but on the other hand this means we will need to redo and force-push the main commit of this PR later.

@Gallaecio
Copy link
Member

probably as a separate PR?

No, we can add it here, and we probably should, we just need to make sure not to squash-and-merge (or rebase-and-merge) the changes, and instead go for a good-old, regular merge commit that does not change the hashes of these commits.

@Gallaecio
Copy link
Member

5. And finally, I feel like we should merge this only after both recent huge PRs (Pylint issues #5677 and use pathlib #5682) are merged, but on the other hand this means we will need to redo and force-push the main commit of this PR later.

That is a very good point 😞

@felipeboffnunes felipeboffnunes mentioned this pull request Nov 24, 2022
@kmike
Copy link
Member

kmike commented Nov 25, 2022

Hey! Based on experience working with https://github.com/scrapy/parsel, it's a bit annoying to have black CI checks, but no easy documented way to configure black, setup the environment.

What do you think about setting up pre-commit and adding instrutions to the docs as a part of this PR? See e.g. https://github.com/scrapinghub/web-poet#developing

@wRAR
Copy link
Member

wRAR commented Nov 26, 2022

The pathlib one is merged and the pylint one is close to be merged so work on this can be restarted quite soon (and in the meantime things that are not related to actually changing the code can be worked on).

@@ -12,7 +15,7 @@ def _indentation_error(*args, **kwargs):

def top_level_return_something():
"""
docstring
docstring
Copy link
Member

Choose a reason for hiding this comment

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

We need to disable this in some way, because these tests are for testing such code.

@Gallaecio
Copy link
Member

the pylint one is close to be merged

Merged now.

@Gallaecio Gallaecio mentioned this pull request Nov 27, 2022
elif response.url.endswith("/general_without"):
self.checks.append(
kwargs == {}
) # pylint: disable=use-implicit-booleaness-not-comparison
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs to be moved to the previous line to work properly.

@wRAR
Copy link
Member

wRAR commented Nov 29, 2022

The changes that remove flake8 errors will most likely be rolled back when you run black again. As I said previously, in this case we will need to silence them in .flake8 (probably same for the remaining pylint ones, I haven't checked the code that was flagged).

ignoring implicit-str-concat pylint error
fixing pylint comment on test_request_cb_kwargs.py
@wRAR
Copy link
Member

wRAR commented Nov 29, 2022

Great!

The next steps are:

  • adding a config for the non-default black options
  • addinfg the CI job, I think it should be done here to prove that the changes here are correct and sufficient
  • adding the pre-commit stuff (maybe as a separate PR)

pyproject.toml Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Member

We also need to add the tox-powered black check to https://github.com/scrapy/scrapy/blob/master/.github/workflows/checks.yml, to have the CI fail when black formatting is not used (e.g. if contributors do not install pre-commit).

@Gallaecio
Copy link
Member

OK, I think the only thing missing is… re-run black now that you removed back the line length setting.

Unless you are comfortable with re-applying everything from scratch as you did in the past, you could alternatively re-run black on a new commit, and add a follow-up commit that marks that second black commit to be ignored by git blame.

@Gallaecio
Copy link
Member

Hmm, the black job is still failing in CI, can you take a look?

@kmike
Copy link
Member

kmike commented Dec 30, 2022

hey! I think we should explain pre-commit in https://docs.scrapy.org/en/master/contributing.html.

docs/contributing.rst Outdated Show resolved Hide resolved
@emarondan emarondan force-pushed the add-black-formatter branch from fe7e3cc to 2888e55 Compare January 18, 2023 14:44
@emarondan emarondan force-pushed the add-black-formatter branch from 2888e55 to 23e8b55 Compare January 18, 2023 14:46
@emarondan emarondan force-pushed the add-black-formatter branch from ffbebde to f2c22aa Compare January 20, 2023 20:08

[testenv:black]
deps =
black==22.12.0
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to do it in this PR, but we have black version pinned in 2 places: tox.ini and pre-commit config. It probably makes sense to just fail the CI if pre-commit run --all-files changes anything, instead of running these tools in tox manually.

Copy link
Member

@kmike kmike left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @emarondan!

@Gallaecio Gallaecio merged commit e71eab6 into scrapy:master Jan 25, 2023
@Gallaecio Gallaecio mentioned this pull request Jan 26, 2023
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 black python formatter Blank lines after import statements
5 participants