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

Consider enforcing flake8, black and isort #401

Open
jezdez opened this issue Oct 18, 2019 · 12 comments
Open

Consider enforcing flake8, black and isort #401

jezdez opened this issue Oct 18, 2019 · 12 comments

Comments

@jezdez
Copy link
Member

jezdez commented Oct 18, 2019

As part of #398 I've added disabled calls to flake8 including a commented-out installation of the flake8-black and flake8-isort plugins.

I would suggest to run black and isort once over the code base and enabling both plugins to have a positive long-term effect on code quality. I didn't make this part of #398 since it's not a hard requirement for Jazzband projects (yet) but I've seen more and more projects move to that strategy.

@claudep
Copy link
Contributor

claudep commented Oct 18, 2019

I'm used to flake8 and isort by working on Django, but I'm not happy at all with black (even if it will come to Django too). At some point, with the accumulation of those code "helper" tools, I'm feeling like that's imposing some sort of "slavery" on developers. However, I understand this is not my project and will accept what the majority prefers.

@hugovk
Copy link
Member

hugovk commented Oct 18, 2019

I like Black, as I feel like it's releasing developers from having to worry about any formatting discussions. Just run a tool and it's done automatically. (Same for isort.)

I've also started using pre-commit too, so you don't need to worry about it. A good thing about that, if you don't like pre-commit, it's completely optional and developers aren't required to use it. And it can be a really handy way to group linters together into a single command on the CI (for example, see linting in tox.ini at https://github.com/hugovk/pypistats/).

@guptarohit
Copy link

I've started using black+flake8+isort with pre-commit hooks in django projects. Things are better now! :)
I second the idea using this combination.

@hugovk
Copy link
Member

hugovk commented Oct 22, 2019

Please see PR #411.

claudep pushed a commit that referenced this issue Oct 22, 2019
This was referenced Nov 10, 2019
claudep added a commit to claudep/tablib that referenced this issue Jan 11, 2020
claudep added a commit to claudep/tablib that referenced this issue Jan 11, 2020
claudep added a commit that referenced this issue Jan 11, 2020
@peymanslh
Copy link
Member

I ran flake8 on the src directory and it raised some issues.
Can I fix some of them and send a PR?

@claudep
Copy link
Contributor

claudep commented Feb 2, 2023

I thought flake8 was run through pre-commit. @hugovk, isn't it the case?

@hugovk
Copy link
Member

hugovk commented Feb 2, 2023

No, we fixed some Flake8 things but because we decided not to use Black there were still some formatting-related ones to fix.

So a PR to fix more would be welcome!

And let's add Flake8 to pre-commit.

We don't necessarily have to fix all the Flake8 right now (but it'd be nice!), we can temporarily exclude some rules and fix those later. It would help prevent new ones slipping in.

@peymanslh
Copy link
Member

Thanks!
I'll add flake8 to pre-commit and fix some issues in 2 separate PRs.
What value should I set for max_line_length?

@claudep
Copy link
Contributor

claudep commented Feb 2, 2023

I like Django's limit (119), but some judge it too high.

@hugovk
Copy link
Member

hugovk commented Feb 2, 2023

That is a bit too high for me :) I often diff two things side by side, that would cause a lot of wrapping.

Shall we use 88? It's also the Black default, to make things easier in case we decide to adopt it in the future?

Still bigger than the Flake8 default 79.

@claudep
Copy link
Contributor

claudep commented Feb 2, 2023

99 and call it a day (I like selling carpets 🤓 )

@hugovk
Copy link
Member

hugovk commented Feb 2, 2023

Deal!

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 a pull request may close this issue.

5 participants