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

Use black python formatter #4658

Closed
imsahil007 opened this issue Jul 5, 2020 · 13 comments · Fixed by #5734
Closed

Use black python formatter #4658

imsahil007 opened this issue Jul 5, 2020 · 13 comments · Fixed by #5734

Comments

@imsahil007
Copy link

Hello Everyone! I hope you are doing okay!
I was looking at the source code and I noticed that the code indentation is poor. So, I suggest using code formatted and create tests for same. I like how "black" works. Tell me what do you think about this?
I am just a beginner to open source. Healthy criticism will be appreciated!

@Gallaecio
Copy link
Member

This is something we have discussed internally, and at the time the consensus was not to switch to Black. But we have been making style improvements to the code base for months.

Most of the code passes Flake8 checks (exceptions are configured in https://github.com/scrapy/scrapy/blob/master/pytest.ini), and we recently merged Pylint checks, which we’ll review with time.

Could you indicate examples of format issues that you’ve found that are not covered by Flake8 or Pylint?

@wRAR
Copy link
Member

wRAR commented Jul 15, 2020

I've just run black -S -l 119 scrapy tests on the master and pushed the result to wRAR@1eadb18 so anyone curious can preview the changes.

@Gallaecio
Copy link
Member

Looks like black does not undo string splits as I would expect: wRAR@1eadb18#diff-bd207f7ef310a40189c53674dde833fdR141

@wRAR
Copy link
Member

wRAR commented Apr 13, 2021

I've updated the branch: wRAR@f842fbe

Previous run: 245 files changed, 3903 insertions(+), 4063 deletions(-)

This run: 253 files changed, 4176 insertions(+), 4115 deletions(-)

@wRAR
Copy link
Member

wRAR commented Jul 26, 2021

Compare: scrapy/cssselect#122 has 4 times fewer changes, though most of that is in three very big files, it would be even easier to review if the changes were split between more files. So it should be possible to reformat the Scrapy repo in several batches if we decide to do that.

@wRAR
Copy link
Member

wRAR commented Jul 26, 2021

My run was without changing the quotes though.

Here is one without -S: wRAR@7fb4d18

284 files changed, 10722 insertions(+), 10635 deletions(-)

Much worse :)

@wRAR
Copy link
Member

wRAR commented Jul 26, 2021

OTOH 70% of that is, curiously, in the tests.

@Gallaecio
Copy link
Member

So it should be possible to reformat the Scrapy repo in several batches if we decide to do that.

A progressive approach works for me, just so that I don’t fall asleep during review. Shall we start with adding black but allow-listing all existing files?

@wRAR
Copy link
Member

wRAR commented Jul 26, 2021

Yeah.

@avishmehta68710
Copy link

Can i work on this issue

@Gallaecio
Copy link
Member

Actually, we had some further internal discussions. It may be best if we handle this internally, to minimize back-and-forth during code review.

@azzamsa
Copy link
Contributor

azzamsa commented Oct 16, 2021

I am very keen to see the project formatted with black. We can have very consistent styling by enforcing it through the CI.

@azzamsa
Copy link
Contributor

azzamsa commented Oct 24, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants